-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Normalize Java import order #6108
Conversation
1241988
to
ce95962
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also include configurations for popular IDEs and verify that they are correct.
We don't want to repeat the mvn formatter:format
situation where CI will reject things that pass ./refine test
if you forget to run mvn formatter:format
by hand.
pom.xml
Outdated
<artifactId>impsort-maven-plugin</artifactId> | ||
<version>1.9.0</version> | ||
<configuration> | ||
<groups>java.,javax.,com.google.refine,org.openrefine,*</groups> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<groups>java.,javax.,com.google.refine,org.openrefine,*</groups> | |
<groups>java.,javax.,*,com.google.refine,org.openrefine</groups> |
Let's use the order from the discussion to minimize the size of the diff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I literally copied it from the discussion, where you wrote:
java javax com.google.refine org.openrefine
But I'll change it to your new version.
So far we have recommended that people import projects via their IDE's Maven support, so I think that in this context the IDE configuration instructions belong to the developer documentation (see OpenRefine/openrefine.org#257). There are attempts to improve IDE support so that those configuration options are automatically detected when importing the project from Maven (https://github.com/revelc/formatter-m2e-configurator, revelc/impsort-maven-plugin#77) but that's work in progress. |
ce95962
to
c8942a4
Compare
Why not store the settings in the project so that they get set up automatically for the developer? That's what we used to do with Eclipse (although the files seem to have been deleted in 2008 c812b61) and I've just confirmed that this is possible for IntelliJ as well. I have on my To-Do list to verify that this works with minimal churn for, at least, IntelliJ, your Maven plugin, and Eclipse with EditorConfig as a stretch goal for those two IDEs. Are there other editors that we should be prioritizing? |
The reason why I deleted those files in the first place is that they were making explicit reference to the Most Java projects I am aware of do not store IDE-specific config files but instead point contributors to the Maven integration of their IDE to get started:
I think not including IDE-specific config files the norm and it intuitively makes sense to me: we only configure our build in one place, in the build system, and IDEs should be able to deal with that. If there is a way to include an IDE-specific configuration, we can consider it, but we'd need to make sure that it does not reference any particular dependency version, any particular JDK, absolute paths or other personal settings which aren't necessarily applicable to everyone. Perhaps that's actually doable. My Eclipse
But if we track this file in the repository, I think it's likely people make commits which accidentally include changes to this file because they have made changes to the configuration of their IDE (we've hade a couple of those in the past, especially before we added |
I accidentally pushed a couple of commits to your branch rather than to a branch in my repo, as intended. I was kind of surprised that it let me, but I suppose they're just as easy for you to look at there and you can remove them if you don't like them. The first commit has IDE configurations for IntelliJ and Eclipse which comes as close as possible to matching what's in your tool and the second commit shows what changes IntelliJ makes with these settings. I think most of them are in the area of wildcard imports. Does the It turns out that EditorConfig doesn't know anything about Java imports, but I included a basic config that has some of our other code style settings in it. |
Thanks for your efforts on this topic. It is normal that you are able to push commits to my branch because this PR is opened with the option to "allow edits and access to secrets by maintainers" (which is turned on by default). I tried re-importing the project in Eclipse with your additional configuration files, but they don't seem to be picked up (I changed my global preferences to use another import order beforehand and checked that this other import order is still used in the freshly created OpenRefine project). Does it actually work on your side? I think we would need to have complete What about IntelliJ - does it successfully pick up the configuration files when importing the project from a fresh clone? Overall I am reluctant to track those editor config files in Git:
So I'd be keen to only keep your
It does not seem to, sadly. It is likely using a fairly basic algorithm which probably isn't able to figure out which classes a wildcard import cover, meaning that it would not be able to normalize a wildcard import back to a list of normal imports. I think it's still valuable to use it to validate the order. |
It looks like the settings need to be set separately for all of our (many) sub-projects, so I've added those. I don't use Eclipse any more and I'd only done a cursory check, so I missed it. Both Eclipse & IntelliJ should work correctly now and match each other. As a bonus, I've added a placeholder for Visual Studio based on this SO post.
I'd prefer to focus on the OpenRefine project's actual needs. The vast majority of projects likely use the default Eclipse order and configure IntelliJ to mimic it. We could switch to that if there's consensus that it's better, but it doesn't seem better to me.
I don't see how this is likely when they are in our
True, but this is an avoidable consequence of the lack of standardization in this area - and it's a static configuration which hasn't changed in over a decade, so it's not like it's an ongoing maintenance cost.
Yes, but those IDEs cover the majority of our developers and there's no downside for someone who doesn't use them The goal is to make it easy for developers to do "the right thing" without having to spend extra time configuring their IDE. It seems like there's a disproportionate amount resistance to this improvement, so if it's just going to be rejected at the end of the day, please let me know and I'll stop wasting time on it. |
I tried again to import the project with Eclipse (version 2023-09) with the additional
Are you following a different procedure? With IntelliJ IDEA, it does seem to work as expected indeed.
I don't know where you see that? I have opened this pull request because you asked for import statement sorting earlier. As stated there, I have no problem with enforcing a particular order. Even though I am not really sure what it brings, I am working on it anyway so as to meet your own needs. If we are to add dozens of config files to enforce this import order, I think it's worth making sure that they are actually picked up by the relevant editors, no? |
I'll take one more crack at fixing the Eclipse settings and then drop it since I don't use Eclipse. Generally things like this work best if they're contributed by regular users of the IDE. I did test with a fresh workspace, but perhaps there's some nuance in how I did the project import. For my purposes, all I care about is IntelliJ, but I'm trying to be a good citizen by investing effort in other popular IDEs, even though I don't use them and won't benefit from the investment. |
I have enabled the removal of unused imports in the Maven plugin, because this is actually supported (but not documented, so I opened revelc/impsort-maven-plugin#80). I'd be happy to remove the Eclipse config files and get this merged, so that we can move on and I can rebase my work on top of that. |
Sorry for the delay. It's probably something trivial, but I don't use Eclipse and no one else seems to care (perhaps no one uses it), so I think it's best to just remove the Eclipse config until there's a developer who has the self-interest to get it right. |
For the record I do use Eclipse as my main IDE |
1e248ee
to
fba71fd
Compare
fba71fd
to
397e83a
Compare
See discussion at https://forum.openrefine.org/t/java-import-ordering/846
A follow-up PR to document the import order on the website will come (see below).