Re: RFR: 8328746: Remove unused imports in demo apps
On Thu, 28 Mar 2024 09:05:00 GMT, Karthik P K wrote: > 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. 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). - PR Comment: https://git.openjdk.org/jfx/pull/1420#issuecomment-2029801487
Re: RFR: 8328746: Remove unused imports in demo apps
On Wed, 27 Mar 2024 14:51:35 GMT, Andy Goryachev wrote: > I would highly suggest to use the current guidelines in CONTRIBUTING.md > > ``` > * Don't worry too much about import order. Try not to change it but don't > worry about fighting your IDE to stop it from doing so. > ``` 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. > as not doing so generates unnecessary (in my opinion) discussion. 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. - PR Comment: https://git.openjdk.org/jfx/pull/1420#issuecomment-2029799109
Re: RFR: 8328746: Remove unused imports in demo apps
On Thu, 21 Mar 2024 21:50:37 GMT, Andy Goryachev wrote: > 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. This PR looks good. - Marked as reviewed by kcr (Lead). PR Review: https://git.openjdk.org/jfx/pull/1420#pullrequestreview-1971320748
Re: RFR: 8328746: Remove unused imports in demo apps
On Thu, 21 Mar 2024 21:50:37 GMT, Andy Goryachev wrote: > 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. 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. > > static imports (sorted alphabetically) > > one blank line > > ordinary imports (sorted alphabetically) > > No wildcard imports (I favor an exception for static imports). > > +1 for something like this. +1 on this and no wildcard imports other than static imports. - PR Comment: https://git.openjdk.org/jfx/pull/1420#issuecomment-2024715556
Re: RFR: 8328746: Remove unused imports in demo apps
On Thu, 21 Mar 2024 21:50:37 GMT, Andy Goryachev wrote: > 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. I would highly suggest to use the current guidelines in CONTRIBUTING.md * Don't worry too much about import order. Try not to change it but don't worry about fighting your IDE to stop it from doing so. as not doing so generates unnecessary (in my opinion) discussion. - PR Comment: https://git.openjdk.org/jfx/pull/1420#issuecomment-2022971911
Re: RFR: 8328746: Remove unused imports in demo apps
On Thu, 21 Mar 2024 21:50:37 GMT, Andy Goryachev wrote: > 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. 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. - PR Comment: https://git.openjdk.org/jfx/pull/1420#issuecomment-2022856424
Re: RFR: 8328746: Remove unused imports in demo apps
On Tue, 26 Mar 2024 16:32:41 GMT, Kevin Rushforth wrote: > static imports (sorted alphabetically) > one blank line > ordinary imports (sorted alphabetically) > > No wildcard imports (I favor an exception for static imports). +1 for something like this. It is a simple rule and pretty sure that it works for all IDEs. Swing is also ordered alphabetically, but with groups usually. Otherwise it looks the order in Swing is the other way around: ordinary imports (sorted alphabetically) one blank line static imports (sorted alphabetically) For me, both are good and I understand and agree to have some king of guideline. That will make reviewing all the import PRs easier as well. Also +1 for no wildcard imports other than static imports. - PR Comment: https://git.openjdk.org/jfx/pull/1420#issuecomment-2022108806
Re: RFR: 8328746: Remove unused imports in demo apps
On Thu, 21 Mar 2024 21:50:37 GMT, Andy Goryachev wrote: > 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. 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. - PR Comment: https://git.openjdk.org/jfx/pull/1420#issuecomment-2021091226
Re: RFR: 8328746: Remove unused imports in demo apps
On Tue, 26 Mar 2024 16:49:58 GMT, Kevin Rushforth wrote: > 1. Should we have at least a soft recommendation or leave it unspecified? 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? - PR Comment: https://git.openjdk.org/jfx/pull/1420#issuecomment-2021042761
Re: RFR: 8328746: Remove unused imports in demo apps
On Tue, 26 Mar 2024 16:38:07 GMT, Andy Goryachev wrote: > > We should replace this with the recommended sort order. > > 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: 1. Should we have at least a soft recommendation or leave it unspecified? 2. If the answer to 1 is "yes", what should that recommendation be? - PR Comment: https://git.openjdk.org/jfx/pull/1420#issuecomment-2020970308
Re: RFR: 8328746: Remove unused imports in demo apps
On Tue, 26 Mar 2024 16:32:41 GMT, Kevin Rushforth wrote: > We should replace this with the recommended sort order. 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). - PR Comment: https://git.openjdk.org/jfx/pull/1420#issuecomment-2020943330
Re: RFR: 8328746: Remove unused imports in demo apps
On Thu, 21 Mar 2024 21:50:37 GMT, Andy Goryachev wrote: > 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. 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: > Don't worry too much about import order. Try not to change it but don't worry > about fighting your IDE to stop it from doing so. 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) one blank line ordinary 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. - PR Comment: https://git.openjdk.org/jfx/pull/1420#issuecomment-2020929321
Re: RFR: 8328746: Remove unused imports in demo apps
On Tue, 26 Mar 2024 16:16:37 GMT, Kevin Rushforth wrote: >> this is how the IDE formatter is currently configured - it should not >> matter, especially since it dos not use wildcards (except for static >> imports). > > 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". It looks like Eclipse uses the following order **by default**: [java,javax,org,com]. - PR Review Comment: https://git.openjdk.org/jfx/pull/1420#discussion_r1539698283
Re: RFR: 8328746: Remove unused imports in demo apps
On Thu, 21 Mar 2024 22:35:22 GMT, Andy Goryachev wrote: >> apps/samples/Ensemble8/src/app/java/ensemble/samplepage/Description.java >> line 55: >> >>> 53: import ensemble.SampleInfo; >>> 54: import ensemble.SampleInfo.URL; >>> 55: import ensemble.generated.Samples; >> >> 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? > > this is how the IDE formatter is currently configured - it should not matter, > especially since it dos not use wildcards (except for static imports). 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". - PR Review Comment: https://git.openjdk.org/jfx/pull/1420#discussion_r1539591609
Re: RFR: 8328746: Remove unused imports in demo apps
On Thu, 21 Mar 2024 22:23:44 GMT, Kevin Rushforth wrote: >> 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. > > apps/samples/Ensemble8/src/app/java/ensemble/samplepage/Description.java line > 35: > >> 33: >> 34: import static ensemble.samplepage.SamplePage.INDENT; >> 35: import static ensemble.samplepage.SamplePageContent.title; > > I see that the static imports (for ensemble.xxx) were moved up to the top. Is > that deliberate? it's how the formatter is currently configured. do we have a preferred order? > apps/samples/Ensemble8/src/app/java/ensemble/samplepage/Description.java line > 55: > >> 53: import ensemble.SampleInfo; >> 54: import ensemble.SampleInfo.URL; >> 55: import ensemble.generated.Samples; > > 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? this is how the IDE formatter is currently configured - it should not matter, especially since it dos not use wildcards (except for static imports). > apps/samples/Ensemble8/src/samples/java/ensemble/samples/graphics2d/puzzle/Piece.java > line 48: > >> 46: /** >> 47: * Node that represents a puzzle piece >> 48: */ > > Hmm. This is more than just fixing imports. Were these lines reformatted > because they were touching the modified import lines? Anything other than > changes to the import statements will require extra time / mental energy to > review. this is just one spot that bothered me. reformatted since I have to delete a blank line. won't happen again, I promise. - PR Review Comment: https://git.openjdk.org/jfx/pull/1420#discussion_r1534768134 PR Review Comment: https://git.openjdk.org/jfx/pull/1420#discussion_r1534769787 PR Review Comment: https://git.openjdk.org/jfx/pull/1420#discussion_r1534771453
Re: RFR: 8328746: Remove unused imports in demo apps
On Thu, 21 Mar 2024 21:50:37 GMT, Andy Goryachev wrote: > 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. I spot-checked a few files and left a couple inline questions. They apply to all of the cleanup PRs. apps/samples/Ensemble8/src/app/java/ensemble/samplepage/Description.java line 35: > 33: > 34: import static ensemble.samplepage.SamplePage.INDENT; > 35: import static ensemble.samplepage.SamplePageContent.title; I see that the static imports (for ensemble.xxx) were moved up to the top. Is that deliberate? apps/samples/Ensemble8/src/app/java/ensemble/samplepage/Description.java line 55: > 53: import ensemble.SampleInfo; > 54: import ensemble.SampleInfo.URL; > 55: import ensemble.generated.Samples; 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? apps/samples/Ensemble8/src/samples/java/ensemble/samples/graphics2d/puzzle/Piece.java line 48: > 46: /** > 47: * Node that represents a puzzle piece > 48: */ Hmm. This is more than just fixing imports. Were these lines reformatted because they were touching the modified import lines? Anything other than changes to the import statements will require extra time / mental energy to review. - PR Review: https://git.openjdk.org/jfx/pull/1420#pullrequestreview-1953540892 PR Review Comment: https://git.openjdk.org/jfx/pull/1420#discussion_r1534760606 PR Review Comment: https://git.openjdk.org/jfx/pull/1420#discussion_r1534761136 PR Review Comment: https://git.openjdk.org/jfx/pull/1420#discussion_r1534762593
RFR: 8328746: Remove unused imports in demo apps
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. - Commit messages: - 8328746: Remove unused imports in demo apps Changes: https://git.openjdk.org/jfx/pull/1420/files Webrev: https://webrevs.openjdk.org/?repo=jfx=1420=00 Issue: https://bugs.openjdk.org/browse/JDK-8328746 Stats: 399 lines in 75 files changed: 98 ins; 189 del; 112 mod Patch: https://git.openjdk.org/jfx/pull/1420.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1420/head:pull/1420 PR: https://git.openjdk.org/jfx/pull/1420
