Change to bespoke spotless import spec to something more typical

[gregschohn]

Description

• Category: Code Cleanup
• Why these changes are required? To make IntelliJ and spotless agree on formatting.
• What is the old behavior before changes and new behavior after changes? Old behavior was that every time I used sensible imports, which is also what IntelliJ was using for me, I'd have to run spotlessApply. Now I don't. I never remembered to run spotlessApply & since it takes 20s in my workflow & spotlessCheck also takes a long time & usually tells me that I have to run spotlessApply - I always do invocations of gradle with -x spotlessCheck. Making this change saves me & other developers the time to have to convert to the bespoke format of imports. Now the import list is something that you don't need to be an 'insider' to understand where to look (i.e. knowing that 3rd party packages could be in two different screens of imports!).

Issues Resolved

No Jira

Is this a backport? If so, please add backport PR # and/or commits #

Testing

gradle & CICD

Check List

• New functionality includes testing
  • All tests pass, including unit test, integration test and doctest
• New functionality has been documented
• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.52%. Comparing base (5709add) to head (3f3b478).
Report is 7 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1048      +/-   ##
============================================
- Coverage     80.55%   80.52%   -0.04%     
+ Complexity     2736     2733       -3     
============================================
  Files           365      365              
  Lines         13614    13614              
  Branches        941      941              
============================================
- Hits          10967    10962       -5     
- Misses         2070     2071       +1     
- Partials        577      581       +4     

Flag	Coverage Δ
gradle-test	78.55% <ø> (-0.05%)	⬇️
python-test	90.11% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[peternied]

Thanks for taking the time to look into these rules, a couple questions about the process to arrive at the ordering, but I'm not directly opposed to these changes.

So long as we've got a way to keep our files looking consistent across the project I think the specific rules choices are less of a concern.

[peternied]

I would recommend that we keep line breaks between external dependencies and internal/java dependencies - the line breaks make it easier to identify if an import is 'ignorable' vs a new dependency or an unexpected dependency.

A quick note on where this preference is from - it makes it easier to review code, as opposed to a preference when writing.

[gregschohn]

I agree. I think that's what the empty '' values were doing. I'd like to bring them in. I wasn't sure if that was getting in the way of static imports getting confused, so I was playing around.
I've never used spotless, so I'm hoping to get some help here.

[gregschohn]

Please take a look at the new version - we have newline/block spaces between groups. spotlessApply with those configs has also DUPLICATED a number of imports. I'm confused at why spotless doesn't consider those duplicate or why it thinks they belong elsewhere. Any guidance would be appreciated?

[peternied]

I've created a PR [1] for spotless that address your duplicate import issue by ignoring duplicate entries, could you take a look?

• [1] Java import order, ignore duplicate group entries diffplug/spotless#2293

[peternied]

Fighting tools is super frustrating - trying to understand your workflow, are you manually placing imports, using auto-add or using the IntelliJ import organizer? If using the import organizer, there are tweaks [1] that can be pulled into it consistent with the existing rules, how does that impact your preference?

• [1] Add IntelliJ formatter #846

[gregschohn]

If you take a look at the comments in the linked PR, you'll see a number of my concerns, that remain unaddressed. I'd like to figure something out to improve this. One issue is in getting IntelliJ to pickup configs by default, which that PR didn't address. Another way to skin that cat would be to use the default IntelliJ list, which is probably more universally adopted than the opensearch one or the one that the spotless rules use now & use that.

The only issue that I have at the moment is that I can't figure out how to get static imports to work well with spotless (they work find w/ IntelliJ).

[peternied]

I'd like consistency when possible. I realize we are divergent from OpenSearch already, but they have a larger and more formal separations declared [1] in their rules. Other than alphabetical (the default ordering) is there a reason for one order vs another?

• [1] https://github.com/opensearch-project/OpenSearch/blob/6020c58659c1a7d8d9a65ae449b6c4b411a74d72/gradle/formatting.gradle#L69-L83

[gregschohn]

This is the rule that I don't understand. Why are some 3rd party packages different than others? e.g. why are google imports going to be in a different block than netty ones? I'd like to see all of those together. That will scale much better over time and doesn't put us into an 'editorial' mode where we're categorizing third party packages.

I'd like to see built-in java imports in one block, optionally by preprocessor blocks (lombok), org.opensearch imports in another, everything else in another, static imports by themselves. Line breaks should be between each of those sections.

I don't think that larger lists are better - it actually means less order - e.g. why is carrotsearch before avast in the linked list? Package names are already naturally grouping when sorted alphabetically by virtue of them being fully-qualified.

The issue that I'm having is that I can't figure out how to even express that with spotless.

[peternied]

you can use an empty string for all the imports you didn't specify explicitly

From https://github.com/diffplug/spotless/tree/main/plugin-gradle#java

I'm guessing using empty string twice is causing those imports to be duplicated when printing, try a single empty string before or after the java|javax matches.

[gregschohn]

Thanks - I put a comment in the gradle file to point to that 'documentation'. It would be awesome for you to open a PR against that repo to improve the documentation. A comment in a code block that describes one behavior and no mention of the '#' doesn't give me warm-fuzzies.

[chelma]

Thanks for the updated import order; I was also confused by the previous ordering but just endured it. This new ordering is much more normal. I appreciate the conversation around norms and standards across the OpenSeach Project, but I think that any external contributors who looked at the code in our repo would find the new ordering much more natural than the Project's "standard" ordering, if that's what we were using before. We can always reassess later if divergence from the "standard" causes problems, but for now it feels like the right path forward.

[peternied]

Please undo the change to the .gitignore - then looks good to me to merge.

[peternied]

Please leave this exclusion in place, vs code puts files in here for individual workspace configuration

[gregschohn]

I can't do that and checkin a file for vscode's autoformatting rules. We had decided that for spotless changes - and spotless - to stay in our processes, that we'd sync changes w/ IDEs so that devs wouldn't have to fight the formatters. Is there another solution?

I would think that users would want to manage their own git ignores locally. Should a repo that doesn't use vscode for the solution to build need to ignore those directories globally (same argument for intelliJ and eclipse)?

[peternied]

Git can still index files that are ignored, I've just moved all my settings from my local workspace to the machine wide configuration, I'll figure this out if it creates friction.