Handle specifically empty filePath, and not default FS rootDir

[blacelle]

I propose this change to handle 2 cases :

1. Exception in formatting while the input content filePath is empty. The javadoc suggests this is legit. However, in case of exception, it generally fails with an technical exception preventing the real functional to be handled.
2. Exception in formatting while the rootDir is not based on the default FileSystem.

It is useful in my case as I consider InMemory files (e.g. based on content from Github API). More specifically, I'm relying on https://github.com/marschall/memoryfilesystem to get InMemory Path (and File("") wherever Spotless requests a File)

if this useCase is considered legitimate, I may open more PR replacing here and there File by Path.

solven-eu/cleanthat#336

[nedtwigg]

I don't like the approach in this PR. I think a better way to solve this problem is to make an actual real file "UnusedFileRequiredForTesting.blah" and pass that file around. That's what we do in StepHarnessWithFile, and the reason we do that is because of confusing bugs we had from passing new File("").

If you are building something where you are running spotless-lib against in-memory filesystems, that's something I'd be happy to support. If you wanted us to change our APIs everywhere from File to Path

spotless/lib/src/main/java/com/diffplug/spotless/FormatterStep.java

Line 46 in 41da6d5

public @Nullable String format(String rawUnix, File file) throws Exception;

I would be open to that. It would have almost no effect on plugin end-users.

The problem is that some formatters really do expect a File on disk. After converting to Path, they would still work fine for Paths on disk, but some of them would fail for e.g. InMemoryFS paths. But we lose no functionality with this change, it's pure gain, just not total gain. It will work for some steps but not others. I think that's okay.

I have two breaking changes to spotless-lib already planned.

• One is a cleanup refactor of the API's that our build plugins use
• obsolete - Internal-only breaking changes #1096
• And the other is changing from throwing exceptions to linting, so that formatters can be specific about the line numbers that they had trouble with
• obsolete - Add support for linters #1097

If you want to do a File -> Path conversion, the time to do it would be after #1096. It's impossible to know and you are always free to change your mind, but how serious are you about needing Path compatibility, and have you checked that the Spotless steps you care about don't actually need a real File?

[blacelle]

I think a better way to solve this problem is to make an actual real file "UnusedFileRequiredForTesting.blah" and pass that file around.

I get the point, but I try not to write any File in given project.

and the reason we do that is because of confusing bugs we had from passing new File("").

I also get this. However, the javadoc suggested new File("") was legitimate (and I initially thought it would be managed specifically).

The problem is that some formatters really do expect a File on disk.

I would be very happy to materialize real File if I needed to rely on such plugins.

It will work for some steps but not others. I think that's okay.

it's okay in my perspective.

how serious are you about needing Path compatibility

Serious enough by dropping my own engine to switch to Spotless :). As my engine essentially consume content by API (and, for instance, try as much as possible not to clone git repository locally), this is a main concern in my perspective. (Also, one major difference with usual spotless useCases is CleanThat would (as primary useCase) not be executed on a personal computer, but in some shared-remote-jvm (e.g. AWS Lambda)).

have you checked that the Spotless steps you care about don't actually need a real File?

My main need is Java formatting. My initial motivation is actually for me to introduce a new plugin into Spotless (some Java refactorer tool). I used to wrap it into a multi-language linter tool (similar to Spotless). I'm currently dropping my linter engine, to rely on Spotless engine (hence planning to push the Java refactorer plugin into Spotless).

-> Eclipse (and most if not all Java spotless formatters) do not need a File. Related formatters (i.e. formatters I used to handle in my legacy linter engine (i.e. json, yaml) do not need File either. I'm very happy with current Formatters non-File coverage. The File -> Path issue seems more related to core-engine than formatters (e.g. like the ones reported here).

[blacelle]

(if I may, I would also suggest relying on new File("someDedicatedMarker"). It may help preventing some issues and difficulties investigating with new File(""))

[nedtwigg]

the javadoc suggested new File("") was legitimate

My memory is that we did have special support for it for the sake of testing, and renounced that with StepHarnessWithFile.

I would also suggest relying on new File("someDedicatedMarker").

I agree with that, iff we have support for some kind of special file. But if it all possible, I would really prefer not to have any magic file mechanism at all.

I'm gonna cut a release tomorrow, and then I'll work on #1096. That will open the door for a File -> Path conversion.

[nedtwigg]

In the meantime, if some kind of magic sentinel file can get you unstuck, I'm okay with merging something well documented along those lines.

[blacelle]

In fact, the code for which I pushed a PR fails due to "" being a relative path. If I pushed new File("/toto"), the 'relativize' may have lead to weird results (in fact, in my case, it would fail as my rootDir is from an inmemoryfilesystem ; for a standard rootDir, .relativize might lead to '..' in the path).

By sentinel, if you refer to a hardcoded File reference, it could be good for me (as I do not foresee the need of an actual File in the shortterm (i.e. I do not rely on Formatters consuming a File for now)). I have a small reserve as my integration is still WIP, and I may encounter more difficulties (e.g. this issue spots due to an Exception bubbling from the Formatter).

[nedtwigg]

By sentinel, if you refer to a hardcoded File reference

I mean something like

public class Formatter {
  public static final File NO_FILE_ON_DISK = new File("NO_FILE_ON_DISK.sentinel")
  
  public void formatOrWhatever(String contentUnix, File file) {
    if (file == NO_FILE_ON_DISK) { // `==` not .equals
      // special handling

I don't love this approach, and I would like to remove it once we have converted from File to Path. But if you are currently blocked and this unblocks you, I'm happy to merge it.

[blacelle]

But if you are currently blocked and this unblocks you, I'm happy to merge it.

I'm currently playing with hacked classes (same name, different jar). It would be helpful to have a cleaner approach before my own release.

I would need both changes (1. to manage an empty/sentinel File, 2. to accept a not-default FileSystem Path). The changes suggested in this PR unblocks with me (i.e. it fixes my issue, it is not just a fix out-of-the-blue). I'm happy to introduce a sentinel File if it can push this PR towards mergeability (as it would also unblock me).

[nedtwigg]

A sentinel File can be merged and released quickly.

The File -> Path change will be weeks slower, a couple weeks probably.

I would like to remove the sentinel after the File -> Path change, but we can cross that bridge when we get there.

[blacelle]

Thanks. This handles one of the 2 items. The other item is managing a custom FileSystem as rootDir. The only difficulty I spotted for now is also in case of exception. Let me talk through the code:

if (file.getPath().isEmpty()) {
	exceptionPolicy.handleError(e, step, "");  // <-- We agree handling this (short-term) with a Sentinel file. I'll adjust the PR soon.
} else {
	String relativePath = rootDir.relativize(rootDir.getFileSystem()
              .getPath(file.getPath())).toString(); // <-- This is a trick to workaround a rootDir with an inmemoryfiesystem: is this also good for you?
	exceptionPolicy.handleError(e, step, relativePath);
}

[nedtwigg]

This is a trick to workaround a rootDir with an inmemoryfiesystem: is this also good for you?

Looks good.

[blacelle]

I introduced a Sentinel, and while scanning for new File(""), I spotted a related Sentinel (I also refactored).

I also fixed the test with custom FileSystem which was broken (code is OK, but the test was not covering the proper lines...)

[nedtwigg]

Missing push?

[jbduncan]

@blacelle @nedtwigg The new File("") is my creation, for better or worse, back from when I was a contributor to Spotless. I don't think I ever explained the rationale behind it; it was meant as an alternative to passing null around to indicate "no file", as I desperately wanted to avoid NullPointerExceptions at the time, but I hadn't considered the consequences of this decision. So if you replaced it with Optional<Path> or a custom OptionalFile type or something, then I wouldn't blame you!

[nedtwigg]

Published in lib 2.34.0, which is not currently used by any of the plugins.

[blacelle]

Thanks a lot @nedtwigg . For the record, I'm left with one class very similar to a Spotless one (but it is not hacked), related to File -> Path: GitAttributesLineEndings -> GitAttributesLineEndings_InMemory

However, the related changes are much deeper than just File -> Path.