8328746: Remove unused imports in demo apps #1420
Conversation
|
👋 Welcome back angorya! A progress list of the required criteria for merging this PR into |
|
@andy-goryachev-oracle This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 13 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
| import static ensemble.samplepage.SamplePage.INDENT; | ||
| import static ensemble.samplepage.SamplePageContent.title; |
There was a problem hiding this comment.
I see that the static imports (for ensemble.xxx) were moved up to the top. Is that deliberate?
There was a problem hiding this comment.
it's how the formatter is currently configured.
do we have a preferred order?
| import ensemble.EnsembleApp; | ||
| import ensemble.PlatformFeatures; | ||
| import ensemble.SampleInfo; | ||
| import ensemble.SampleInfo.URL; | ||
| import ensemble.generated.Samples; |
There was a problem hiding this comment.
I see that ensemble.xxx is moved after javafx.xxx; similarly, elsewhere I see com.sun.xxx imports moved after. What's the algorithm your IDE uses to sort imports? sort stuff that starts with "java" first and then everything else alphabetically?
There was a problem hiding this comment.
this is how the IDE formatter is currently configured - it should not matter, especially since it dos not use wildcards (except for static imports).
There was a problem hiding this comment.
While it doesn't matter from a correctness point of view, if we are going to make changes like this to several files across multiple PRs, I want to make a deliberate decision rather than "whatever the Eclipse IDE happens to be configured to do by default". That way whenever new files are created or existing files are modified, we can have consistency rather than "dueling import sorting".
There was a problem hiding this comment.
It looks like Eclipse uses the following order by default: [java,javax,org,com].
apps/samples/Ensemble8/src/samples/java/ensemble/samples/graphics2d/puzzle/Piece.java
Show resolved
Hide resolved
|
Here is my $0.02 on the sorting of imports. I haven't cared enough before now to raise the issue, but since we are now going to be setting a precedent with these PRs, I want to codify the style guidelines. We should update the CONTRIBUTING guidelines to reflect whatever we decide. The current guidelines say this:
We should replace this with the recommended sort order. As for what that order should be, I have never much liked the idea of sorting or organizing imports into multiple groups with a set of rules that you have to remember / describe / teach your IDE about. The various IDEs or code-style guidelines with such conventions are often at odds with each other. Google used to have multiple groups (with rules about sorting the groups), but finally tired of this and changed to a simpler convention of having two groups: one for static imports (if any) and one for ordinary imports: static imports (sorted alphabetically) No wildcard imports (I favor an exception for static imports). I propose that we do the same, but if someone has strong opinions otherwise, we can discuss. |
My 2 cents would be on the other side: I would strongly recommend against enforcing a specific order, especially since it causes no issues. We should avoid wildcards, yes (except for static imports), but the order - we should follow the current recommendation of ignoring the order. The main reason it is adding zero value and costs some time (just like whitespace warnings in jcheck). |
I agree that we shouldn't enforce the order (and didn't mean to imply otherwise). I do see some value in having a "soft" recommendation, though, and encouraging IDE users to configure their IDEs with those recommendations. Otherwise, it can be a source of merge conflicts if one IDE sorts it one way, and another IDE sorts it a different way, and two different people are proposing changes to the same file. Maybe unlikely, but it has happened. So the questions are:
|
I would rather leave it unspecified. My concern is that different IDEs might have different sort orders by defaul (now or in the future) and then we have a perpetual problem. (Disclosure: I don't actually know the default order in IntelliJ/Netbeans/VS). The merge conflicts in the imports are easy to resolve and are possible even when the order is the same, so it should not be a deciding factor. What do other people think? |
|
The other thing that a reordering of imports does is add noise to a review, so there is some advantage in having it be at least reasonably consistent. The current advice is already "try not to reorder imports", so as long as people follow that, it might be good enough. What I don't want to see is a lot of back and forth reordering, but maybe it won't be a problem (it typically hasn't been up to now). I don't have a strong opinion on this, other than the "no wildcard imports" rule, which is separate and already documented. I do like consistency, but as long there is some reasonable ordering, I can let it go if no one else feels the need for a gudeline. |
+1 for something like this. It is a simple rule and pretty sure that it works for all IDEs. For me, both are good and I understand and agree to have some king of guideline. Also +1 for no wildcard imports other than static imports. |
|
I find it helpful to separate the imports by their high-level domain name, "java.", "javafx.", "com." etc. It makes it easier to see the "span" or requirements of the class. |
|
I would highly suggest to use the current guidelines in CONTRIBUTING.md as not doing so generates unnecessary (in my opinion) discussion. |
|
Leaving it unspecified would be ok but some kind of soft recommendation could also help in my opinion. So there will be a direction for someone who is looking for a recommended way to add imports and have consistency class files.
+1 on this and no wildcard imports other than static imports. |
Since this is the existing guideline, and there isn't clear agreement on a new guideline, this seems like the best approach. We can revisit this after the current set of PRs are integrated if there is a desire to do so.
And I think that it is the absence of at least a soft guideline that generates the discussion, but we can table the discussion for now. |
This might be the best approach. A soft recommendation in addition to the current advice to try not to reorder the imports in connection with bug fixes. We can discuss on the list (decoupled from this PR). |
|
/integrate |
|
Going to push as commit 01a1d90.
Your commit was automatically rebased without conflicts. |
|
@andy-goryachev-oracle Pushed as commit 01a1d90. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Using Eclipse IDE to remove unused imports in demo apps (3D, Ensemble, etc.) and update the copyright year to 2024. Using wildcard for more than 10 static imports.
--
This is a trivial change (though fairly large), 1 reviewer is probably enough.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1420/head:pull/1420$ git checkout pull/1420Update a local copy of the PR:
$ git checkout pull/1420$ git pull https://git.openjdk.org/jfx.git pull/1420/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1420View PR using the GUI difftool:
$ git pr show -t 1420Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1420.diff
Webrev
Link to Webrev Comment