ImportSorter eats comments #558

Activity

1. nedtwigg

   added

   bug

   importsorter

   on Apr 23, 2020
2. nedtwigg commented on Apr 23, 2020

   

   nedtwigg

   on Apr 23, 2020

   Member

   More actions

   If a comment is to the right of an import, it's clear who it should get sorted with. But for your example, should it count as above its target or below? One possible option:

   importSorter(COMMENT_SORTWITH_ABOVE)
   importSorter(COMMENT_SORTWITH_BELOW)
   importSorter(COMMENT_REMOVE) // the current behavior
   importSorter(COMMENT_ERROR)  // a safer default

   IMO "no comments in the imports" is a reasonable styleguide. Our importsorter has several issues, but I wonder how Eclipse or IntelliJ handle this. Happy to take a PR to improve this behavior, but it's extremely low on my todo list.
3. jochenberger commented on Apr 24, 2020

   

   jochenberger

   on Apr 24, 2020

   ContributorAuthor

   More actions

   Yes, I can see how one would consider comments in between import statements bad style or practice, but in our case, we need them as markers during code generation.
   The current behavior however is a problem in my opinion, because a sort step should not remove any code, not even comments. It seems as if you agree, since you added the bug label. :-)
   Most (if not all) of the code I know seems to treat single-line comments as belonging to the line (or, more generally speaking, code) below.
   However, it just came to my mind that considering the comment to belong to the line below wouldn't actually help with our current use case.
   I already said that we generate code. There's a mechanism to be able to add custom code that is left alone when re-generating. Our imports look like this:

   package org.example;
   
   import java.util.List;
   import java.util.Set;
   // begin user-defined imports
   import java.util.ArrayList;
   import org.example.PersonUtilities;
   // end user-defined imports
   
   public class Test {
     ....
   }

   Just treating the comments as a two-line import statement and using the regular import sorting won't help because it would move the List and Set imports into the user-defined imports.
   So, I can think of three solutions that would help us here, ordered from the (IMO) worst to best:

   • being able to specify different steps for different targets, i.e. exclude the import sorter for our generated sources
   • being able to specify importSorter(COMMENT_NOOP) to just skip the sorting if comments are found
   • Separating the FormatterStep from the sorter, i.e. extract the actual sorting into a separate class that you can override, either with a class on the classpath (importSorterCustom(org.example.CustomSorter)) or even inline (importSorterCustom({List<String> lines-> lines.reverse()} as com.diffplug.spotless.java.IImportSorter))

   I can imaging us helping with a PR if we settle on a desired solution.
4. nedtwigg commented on Apr 24, 2020

   

   nedtwigg

   on Apr 24, 2020

   Member

   More actions

   being able to specify different steps for different targets, i.e. exclude the import sorter for our generated sources

   This is very easy, just one piece missing. If we make this method public:

   spotless/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessExtension.java

   Line 217 in 608af5c

   private <T extends FormatExtension> void configure(String name, Class<T> clazz, Action<T> configure) {

   then you could do this:

   spotless {
     java {
       targetExclude 'src/main/java-autogenerated/**'
       googleJavaFormat()
       importSorter()
     }
     configure 'javaAutogenerated', com.diffplug.gradle.spotless.JavaExtension, {
       target 'src/main/java-autogenerated/**'
       googleJavaFormat()
     }
   }
   

   Should probably rename "configure" to "format".

   being able to specify importSorter(COMMENT_NOOP) to just skip the sorting if comments are found

   COMMENT_NOOP is a good idea - easier to implement than COMMENT_SORTWITH_ABOVE/SORTWITH_BELOW.

   Separating the FormatterStep from the sorter, i.e. extract the actual sorting into a separate class that you can override, either with a class on the classpath

   I think it would be better to just copy-paste the existing sorter and modify to make a custom formatter step, rather than burden the import sorter with a public API to maintain for a very niche usecase.

   A PR for any viable solution is welcome.
5. jochenberger commented on Jun 17, 2020

   

   jochenberger

   on Jun 17, 2020

   ContributorAuthor

   More actions

   This issue is no longer relevant to us, so feel free to close it.

   👍React with 👍1Reacted by Ned Twigg
6. nedtwigg

   closed this as completedon Jun 18, 2020