Community
Participate
Working Groups
I suggest to update the default clean-up profile. I think we should add: Organize imports Use always blocks Convert to lambdas Remove redundant type arguments
Setting the milestone so that this is not forgotten. I'm happy to contribute a patch if the JDT agrees to a set of changes.
Also: Remove trailing whitespace (o) all lines Convert for loops to enhanced Convert modifier final where possible [x] private fields
(In reply to Andrey Loskutov from comment #2) > Remove trailing whitespace (o) all lines > Convert for loops to enhanced +1 > Convert modifier final where possible [x] private fields -1 for enabling "Use modifier "final" where possible for private fields. With Java 8 (effective final) this is even less important and IMHO using "final" unnecessary clutters the code. Also the default Eclipse cleanup profile should reflectd the Eclipse programming standards in which we also do not use final if not necessary.
I think this has been mentioned in the past. I am not in favor of this kind of changes that will introduce unrelated changes to the commit. Not just that, it will affects the history and in some cases, makes the blame annotations almost useless.
(In reply to Jay Arthanareeswaran from comment #4) > I think this has been mentioned in the past. I am not in favor of this kind > of changes that will introduce unrelated changes to the commit. Not just > that, it will affects the history and in some cases, makes the blame > annotations almost useless. I guess you misunderstood the request. This is not about "automated fix" on save, this is about Source -> Cleanup defaults, which is not applied automatically.
(In reply to Andrey Loskutov from comment #5) > (In reply to Jay Arthanareeswaran from comment #4) > > I think this has been mentioned in the past. I am not in favor of this kind > > of changes that will introduce unrelated changes to the commit. Not just > > that, it will affects the history and in some cases, makes the blame > > annotations almost useless. > > I guess you misunderstood the request. This is not about "automated fix" on > save, this is about Source -> Cleanup defaults, which is not applied > automatically. I was wondering exactly this question. I'm relieved to see that this is not about Save Actions, but then: when will those cleanups be performed? Only for new files? That's the only viable strategy that I could imagine.
(In reply to Stephan Herrmann from comment #6) > but then: when will those cleanups be performed? Only > for new files? That's the only viable strategy that I could imagine. I am not sure if that works only on new files. If it touches even existing files, then, for e.g., removing trailing whitespace might land us in trouble.
Jay/Stephan, this bug is about adjusting the default profile, not about applying it to JDT core. Please check yourself via the context menu, Source-> Cleanup
(In reply to Lars Vogel from comment #8) > Jay/Stephan, this bug is about adjusting the default profile, not about > applying it to JDT core. > > Please check yourself via the context menu, Source-> Cleanup In that case the bug is only placed in the wrong bucket. Moving to where default cleanup profiles are *implemented*. JDT/Core has nothing to do with that. See org.eclipse.jdt.internal.corext.fix.CleanUpConstants.setEclipseDefaultSettings(CleanUpOptions)
Moving to M7.
New Gerrit change created: https://git.eclipse.org/r/142480
I suggest to also add the "Remove redundant Semicolons"
Like with formatter defaults we can't change them under the feet of our developers and customers, especially because that affects their code. When we modernized the Formatter profile we introduced a new one. This is what we can do here too. We can either come up with a new name for the new profile or reuse the existing name and rename the old one, e.g. "Eclipse < 4.13 [built-in]". For the formatter we did the latter. As for the suggested changes, I agree with them except with Use always blocks This has nothing to do with "modern", it's just a pure style issue. Lars, I happily saw that you did not include this in your suggested Gerrit change. We should not enable anything on the 'Code Style' tab.
Moving out of 4.13. Target can be reassigned when updated patch is available.
(In reply to Dani Megert from comment #13) > Like with formatter defaults we can't change them under the feet of our > developers and customers, especially because that affects their code. When > we modernized the Formatter profile we introduced a new one. This is what we > can do here too. Actually we did not. We changed the default formatter and the agreement was that this is OK. IIRC if we change the defaults and the customer used the default profile as save action nothing will change automatically, he has to manually reselect the profile. > We can either come up with a new name for the new profile or reuse the > existing name and rename the old one, e.g. "Eclipse < 4.13 [built-in]". For > the formatter we did the latter. > As for the suggested changes, I agree with them except with > Use always blocks > This has nothing to do with "modern", it's just a pure style issue. +1 Lars, I > happily saw that you did not include this in your suggested Gerrit change. > We should not enable anything on the 'Code Style' tab.
Dani, please response or remove your -1 in the change.
(In reply to Lars Vogel from comment #15) > (In reply to Dani Megert from comment #13) > > Like with formatter defaults we can't change them under the feet of our > > developers and customers, especially because that affects their code. When > > we modernized the Formatter profile we introduced a new one. This is what we > > can do here too. > > Actually we did not. We changed the default formatter and the agreement was > that this is OK. IIRC if we change the defaults and the customer used the > default profile as save action nothing will change automatically, he has to > manually reselect the profile. If the Clean Up settings are not stored in the project and we change the default, then running the Clean Up will change the code. I can be convinced to accept that as long as we document this in the N&N and give the advice to store the Clean Up settings in the project. WDYT?
Fabrice, which cleanups would you recommend to have enabled by default?
Here is my list and then my arguments: As "Save action": ELSE_IF NUMBER_SUFFIX As "Cleanup": USE_SWITCH PULL_UP_ASSIGNMENT ELSE_IF REDUCE_INDENTATION INSTANCEOF NUMBER_SUFFIX SIMPLIFY_LAMBDA_EXPRESSION_AND_METHOD_REF PRECOMPILE_REGEX NO_STRING_CREATION PREFER_BOOLEAN_LITERAL SINGLE_USED_FIELD BREAK_LOOP STATIC_INNER_CLASS STRINGBUILDER PLAIN_REPLACEMENT USE_LAZY_LOGICAL_OPERATOR VALUEOF_RATHER_THAN_INSTANTIATION PRIMITIVE_COMPARISON PRIMITIVE_PARSING PRIMITIVE_SERIALIZATION INSERT_INFERRED_TYPE_ARGUMENTS SUBSTRING ARRAYS_FILL EVALUATE_NULLABLE PUSH_DOWN_NEGATION DOUBLE_NEGATION REMOVE_REDUNDANT_COMPARISON_STATEMENT REDUNDANT_SUPER_CALL UNREACHABLE_BLOCK REDUNDANT_FALLING_THROUGH_BLOCK_END COLLECTION_CLONING MAP_CLONING OVERRIDDEN_ASSIGNMENT REMOVE_REDUNDANT_MODIFIERS RAISE_EMBEDDED_IF REMOVE_REDUNDANT_SEMICOLONS REDUNDANT_COMPARATOR REMOVE_UNNECESSARY_ARRAY_CREATION ARRAY_WITH_CURLY REMOVE_USELESS_RETURN REMOVE_USELESS_CONTINUE UNLOOPED_WHILE FORMAT_SOURCE_CODE ORGANIZE_IMPORTS SORT_MEMBERS MODERNIZE_HASH USE_OBJECTS_EQUALS OPERAND_FACTORIZATION TERNARY_OPERATOR STRICTLY_EQUAL_OR_DIFFERENT MERGE_CONDITIONAL_BLOCKS CONTROLFLOW_MERGE USE_PATTERN_MATCHING_FOR_INSTANCEOF CONTROL_STATEMENTS_CONVERT_TO_SWITCH_EXPRESSIONS USE_VAR USE_LAMBDA COMPARING_ON_CRITERIA JOIN TRY_WITH_RESOURCE MULTI_CATCH REMOVE_REDUNDANT_TYPE_ARGUMENTS USE_AUTOBOXING USE_UNBOXING CONTROL_STATEMENTS_CONVERT_FOR_LOOP_TO_ENHANCED CONTROL_STATEMENTS_CONVERT_FOR_LOOP_ONLY_IF_LOOP_VAR_USED CONTROL_STATEMENTS_USE_ADD_ALL Firstly, I exclude all the "Source fixing" cleanups as it may alter the code. For the same reason, I exclude the cleanups that are based on methods as a custom implementation may behave in an unexpected way. I include and exclude some "Code style" cleanups as someones are common conventions and others are just suggestion but not a common convention. For the rest, we should include as many cleanups as possible. Only few developers use the cleanups because you need to discover the feature. The more you have to click, the less a person discover it. I include very few things in "Save action" as it apply on unfinished code and cleanups need to run on valid code. The ones I added are almost formatting.
New Gerrit change created: https://git.eclipse.org/r/c/jdt/eclipse.jdt.ui/+/178732
Please enable only those clean ups that were discussed before comment #17 in this bug. If you want to include any other clean ups then provide an explanation/justification for each of them. Also, make sure to document the effect and guidelines as mentioned in comment #17.
(In reply to Fabrice Tiercelin from comment #19) If you plan to enable all these clean ups then please add a new experimental profile for that in a separate bug report. I do not recommend enabling all these by default as many of them are still incomplete with follow up bugs being discovered or worked upon. Since clean ups are applied as bulk operation and can potentially cause semantic bugs in the code that can go unnoticed, they should be enabled only after they are more stable than the current state.
OK so I have restored all the cleanups that are new in Eclipse 2021-03 and the ones that have or had recently open bugs. For the ones added after comment #17, I will add comment after this message.
NUMBER_SUFFIX An old cleanup without bug since. A simple cleanup close to a format rule. PRECOMPILE_REGEX From Eclipse 4.17 without bug since. NO_STRING_CREATION From Eclipse 4.18 without bug since. Very local changes. PREFER_BOOLEAN_LITERAL From Eclipse 4.18 without bug since. Very local changes. SINGLE_USED_FIELD From Eclipse 4.18 without bug since. USE_LAZY_LOGICAL_OPERATOR An old cleanup without bug since. A simple cleanup close to a format rule. PRIMITIVE_COMPARISON From Eclipse 4.19 without bug since. Very local changes. PRIMITIVE_PARSING From Eclipse 4.19 without bug since. Very local changes. PRIMITIVE_SERIALIZATION From Eclipse 4.19 without bug since. Very local changes. INSERT_INFERRED_TYPE_ARGUMENTS An old cleanup without bug since. A simple cleanup close to a format rule. EVALUATE_NULLABLE From Eclipse 4.18 without bug since. Very local changes. PUSH_DOWN_NEGATION An old cleanup without bug since. DOUBLE_NEGATION From Eclipse 4.18 without bug since. Very local changes. REMOVE_REDUNDANT_COMPARISON_STATEMENT From Eclipse 4.18 without bug since. Local changes. REDUNDANT_SUPER_CALL From Eclipse 4.18 without bug since. Very local changes. REDUNDANT_FALLING_THROUGH_BLOCK_END From Eclipse 4.18 without bug since. It demonstrates the power of Eclipse. COLLECTION_CLONING From Eclipse 4.18 without bug since. Local changes. MAP_CLONING From Eclipse 4.18 without bug since. Local changes. REMOVE_REDUNDANT_MODIFIERS From Eclipse 4.16 and before without bug since. Very local changes. RAISE_EMBEDDED_IF From Eclipse 4.18 without bug since. Local changes. REMOVE_REDUNDANT_SEMICOLONS An old cleanup without bug since. A simple cleanup close to a format rule. REMOVE_UNNECESSARY_ARRAY_CREATION An old cleanup without bug since. REMOVE_USELESS_RETURN From Eclipse 4.18 without bug since. Local changes. REMOVE_USELESS_CONTINUE From Eclipse 4.18 without bug since. Local changes. FORMAT_SOURCE_CODE An old cleanup without bug since. A simple cleanup close to a format rule. ORGANIZE_IMPORTS An old cleanup without bug since. A simple cleanup close to a format rule. SORT_MEMBERS An old cleanup without bug since. A simple cleanup close to a format rule. MODERNIZE_HASH From Eclipse 4.18 without bug since. It demonstrates the power of Eclipse. USE_OBJECTS_EQUALS From Eclipse 4.18 without bug since. It demonstrates the power of Eclipse. TERNARY_OPERATOR From Eclipse 4.18 without bug since. STRICTLY_EQUAL_OR_DIFFERENT From Eclipse 4.18 without bug since. MERGE_CONDITIONAL_BLOCKS From Eclipse 4.18 without bug since. USE_PATTERN_MATCHING_FOR_INSTANCEOF From Eclipse 4.18 without bug since. It demonstrates that Eclipse is modern. CONTROL_STATEMENTS_CONVERT_TO_SWITCH_EXPRESSIONS From Eclipse 4.18 without bug since. It demonstrates that Eclipse is modern. USE_VAR It demonstrates that Eclipse is modern. USE_LAMBDA It demonstrates that Eclipse is modern. COMPARING_ON_CRITERIA It demonstrates that Eclipse is modern. JOIN It demonstrates that Eclipse is modern. TRY_WITH_RESOURCE It demonstrates that Eclipse is modern. MULTI_CATCH It demonstrates that Eclipse is modern. REMOVE_REDUNDANT_TYPE_ARGUMENTS An old cleanup without bug since. A simple cleanup close to a format rule. USE_AUTOBOXING An old cleanup without bug since. A simple cleanup close to a format rule. USE_UNBOXING An old cleanup without bug since. A simple cleanup close to a format rule. CONTROL_STATEMENTS_CONVERT_FOR_LOOP_TO_ENHANCED An old cleanup without bug since. A simple cleanup close to a format rule. CONTROL_STATEMENTS_CONVERT_FOR_LOOP_ONLY_IF_LOOP_VAR_USED An old cleanup without bug since. A simple cleanup close to a format rule. CONTROL_STATEMENTS_USE_ADD_ALL An old cleanup without bug since. A simple cleanup close to a format rule.
This was discussed during JDT call and the following two options were proposed: 1) Enable only those clean ups that were discussed until comment #17 in the default profile. Create a new clean up profile "experimental" (or another name) for clean ups in comment #24 via a separate bug. Or, 2) Enable only the most relevant and useful new clean up options (approx. 4-5) in the default profile. For that, filter the list of proposed clean ups (comment #24): - Remove any non-relevant options (e.g. Code style clean ups as these are just personal style preferences not to be enforced in the defaults). - Apply each clean up option to the Eclipse SDK (Platform+JDT+PDE) codebase and check if there are any issues and the whether the test cases pass. - Vote for the most useful clean up options out of the remaining ones to be included in the defaults. Jeff can help with evaluating them.
Gerrit change https://git.eclipse.org/r/c/jdt/eclipse.jdt.ui/+/178732 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=186b204fb4ef60b465039b1ff18ec2b7df68f995
I'm not sure how this works but I guess we have to implement something in public class CleanUpProfileVersioner implements IProfileVersioner { public static final String PROFILE_KIND= "CleanUpProfile"; //$NON-NLS-1$ private static final int VERSION_1= 1; // 3.3M2 private static final int VERSION_2= 2; // 3.3M3 Added ORGANIZE_IMPORTS public static final int CURRENT_VERSION= VERSION_2; @Override public int getFirstVersion() { return VERSION_1; } @Override public int getCurrentVersion() { return CURRENT_VERSION; } @Override public void update(CustomProfile profile) { final Map<String, String> oldSettings= profile.getSettings(); Map<String, String> newSettings= updateAndComplete(oldSettings, profile.getVersion()); profile.setVersion(CURRENT_VERSION); profile.setSettings(newSettings); } This is in org.eclipse.jdt.internal.ui.preferences.cleanup of org.eclipse.jdt.ui and seems to be responsible to adjust cleanup profiles.
@Fabrice, please add the N&N and close any pending issues here for M3 by EOD.
New Gerrit change created: https://git.eclipse.org/r/c/jdt/eclipse.jdt.ui/+/179637
New Gerrit change created: https://git.eclipse.org/r/c/www.eclipse.org/eclipse/news/+/180781
Gerrit change https://git.eclipse.org/r/c/www.eclipse.org/eclipse/news/+/180781 was merged to [master]. Commit: http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=d3fe08c2152b4d3d333b469f17afbff61875ffb7
(In reply to Noopur Gupta from comment #28) > @Fabrice, please add the N&N and close any pending issues here for M3 by EOD. Done on behave of Fabrice. Thanks Fabrice for the work.
(In reply to Lars Vogel from comment #32) > (In reply to Noopur Gupta from comment #28) > > @Fabrice, please add the N&N and close any pending issues here for M3 by EOD. > > Done on behave of Fabrice. > > Thanks Fabrice for the work. Thank you, I have forgotten this N&N.
