Import Sorter to cope with multi-line comments and misplaced imports

[fvgh]

The change shall enhance the robustness of the java.ImportOrderStep step.
I admit that I basically need it to use it on Groovy code (see #13), but it is also an issue in the java world:

1. If you comment out imports, the java.ImportOrderStep step will ignore the multi-line comment and use these imports in its sorted list

2. If you have misplaced imports (still working on code, or using groovy), the java.ImportOrderStep will (delete all code in between)[java.ImportOrderStep].

Two improvements:

1. The ImportSorter stops looking for import statements, as soon as it finds the beginning of a scope.
2. The ImportSorter gets a rough idea what multi-line comments are, so that it does not look for tokens ({, import) within multi-line comments.

[nedtwigg]

I looked at this briefly. The modifications look good, but the original code is kinda spaghetti so I'm not sure if it introduced a subtle bug or not (maybe even fixed some, who knows!).

For code like this, it earns my trust by simple mileage - run it on a ton of code and make sure nothing breaks. Once this gets merged to master so that it shows up in -SNAPSHOT, then I'll test this on my work codebases to make sure it doesn't introduce any subtle bugs before we release.

@jbduncan tends to have a more careful eye than me, so I'll defer to him :)

[fvgh]

I was wondering whether it is time to put the format method into a child class, but since this means a complete refactoring and it would make it even harder to compare the original code with the few modifications. Your choice, let me know...

I ran the new version on junit, for which I need it in the first place.

[nedtwigg]

I ran the new version on junit, for which I need it in the first place.

Fantastic! Good enough for me 👍

I was wondering whether it is time to put the format method into a child class

The problem with naive pseudo-parsers such as ImportSorter is that they're easy to get working for most cases, but they suck up a lot of time with obscure bugs later in their lifecycle. I absolutely don't think it's worth a full rewrite - it's worked great so far, and you've tested it well.

I think you made the right call re: refactoring.

[fvgh]

That is also the point why I did not open a new child-class. You can of course implement more and more features to come closer to a real syntax aware implementation. But this cannot be the goal.
As I stated in my code comments, the multi-line detection is not complete.
But also the detection of import is not complete since it only accepts the import statement at the very beginning of the line, which is not in line with the Java syntax definition.
As long as we can cope with sensible code, that should be good enough. I was just a little bit surprised when the ImportOrder eat half of my groovy file due to scattered imports.
Here again, my solution is not perfect. The solution will only sort the imports for the first class. If the second class in the same file has additional imports, it will not touch them.
But for me that is good enough. Having multiple classes in one file is a feature in Groovy which in some cases can make your code readable. Scattering the imports in your code is in very rare cases useful, but then just when you need just one or two more imports. Other wise you should really ave two files for two classes. However, in a clean-code scenario I do not need a sorting for these additional imports.

[jbduncan]

Although I do have a comment about a code block within ImportSorter.java, it's not clear to me that this change is wise.

google-java-format, which Spotless supports, purposely throws an exception upon encountering comments within the imports section, since it's not clear whether the comment was purposely inserted or not, since it may document something important, so it should be up to a human to decide whether it should be removed or placed elsewhere in the file.

[jbduncan]

I'm not entirely sure that this if..continue block is needed.

AFAICT, !next.contains("/*") can never be true at this point. I can explain my reasoning if you wish, but it's a bit long-winded, I've not tested it with JUnit or a debugger, and there's no harm to this block staying anyway AFAICT.

[fvgh]

You have this situation in the following case:

/* //isMultiLineComment = true
import what.so.ever */ //isMultiLineComment = false

In the old code what.so.ever would have been part of the refactored imports, so it was originally commented out.
If I refactor it to an inner class, it becomes easier to read. But at first I wanted to assure you, that I actually did not alter much in the initial logic of the method.

[jbduncan]

In the old code what.so.ever would have been part of the refactored imports, so it was originally commented out.

Sorry, I don't understand what this sentence is saying. Can you rephrase it for me?

[fvgh]

Original behavior:

import i.need.this
/*
import i.don.t.need.that */

Results in

import i.need.this
import i.don.t.need.that

New behavior detects multi-line comments. So result is:

import i.need.this

[fvgh]

Sorry, just saw my mistake. I wanted to say:
In the old code what.so.ever would have been part of the refactored imports, though it was originally commented out.

[jbduncan]

Aha I see now. Thanks for the clarifications @fvgh. :)

[jbduncan]

Hi @fvgh, as I mentioned in my review, it's not clear to me that this code change is wise.

google-java-format, which Spotless supports, purposely throws an exception upon encountering comments within an imports section, since it's not clear whether the comment was purposely inserted or not, and it may document something important. So it makes some sense to me that it should be up to a human to decide whether it should be removed or placed elsewhere in the file, rather than let Spotless indiscriminately remove them.

But then again, most users (if not all users) of Spotless will also be using a VCS like Git, so they can revert or change commits if an important comment was accidentally removed. So maybe this isn't such a big problem... 🤔

[fvgh]

throws an exception upon encountering comments within the imports section

This is an option. I thought that the current behavior to eat comments was intended, so I did not change it. But let's do that in a separated PR, since it is unrelated to my changes.

they can revert or change commits

I think if you run a code formatter over your complete code without source control, well, ...

Refactoring the code and use an inner class is definitely an option here. But in the current form I think it is easier to see what I changed in the behavior. So for me it is important that we have both a common understanding of the behavioral change before I propose a refactored code.
The unit test files I provided should give a compressed view on the changed behavior. Once we agreed on it, I can offer you, as a second non-squashed commit, a proposal for an inner class solution.
I trust my UT to guarantee that I don't destroy anything. As regression test for the original behavior I will check with JUnit again.
Let me know if you have more questions on the logic and afterwards I can provide you with a refactored code if you like.

[jbduncan]

I thought that the current behavior to eat comments was intended, so I did not change it.

Aha, I didn't realise that. Let's keep the comment-eating behaviour then.

I'm fine with the tests, and it looks like they pass, so no problems there.

[jbduncan]

Basically, this LGTM now. 👍