eclipse-mylyn / org.eclipse.mylyn

Eclipse Public License 2.0
13 stars 9 forks source link

Create Java->Code Style configuration for Java 17 and other best practices #200

Closed gnl42 closed 10 months ago

gnl42 commented 1 year ago

The Mylyn workspace is using the default Eclipse 'Code Style' settings for 'Code Cleanup' and 'Formatting' (which seem to be stuck in the JavaSE-1.5 era ???)

With Java17 now the default for the project there are a lot of settings that we can take advantage of to create cleaner code.

gnl42 commented 1 year ago

I exported CleanUp and Formatter locally but GitHub won't allow me to add to the issue *&^%^(

I don't know the Eclipse internals enough as how to set up the projects to use them though.

@BeckerFrank Not sure where to go from here??

BeckerFrank commented 1 year ago

I exported CleanUp and Formatter locally but GitHub won't allow me to add to the issue *&^%^(

I don't know the Eclipse internals enough as how to set up the projects to use them though.

@BeckerFrank Not sure where to go from here??

I think we have the settings on the single project in the .settings folder. Can you place the exported file in the project org.eclipse.mylyn.releng so that I can try to import the file and look how we can set it for all projects.

merks commented 1 year ago

Are you trying to create project-specific settings? That’s a lot of copying to manage!

This can also be set globally but then we would want to do that in the Oomph setup so that we all have it configured the same way.

Will this change the save options such that any file we touch will be formatted with new formatting?

merks commented 1 year ago

Project-specific settings are best because there is no special setup required. But keeping them consistent is not trivial.

gnl42 commented 1 year ago

I created CodeStyleTemplates folder in org.eclipse.mylyn.releng in branch 200_CodeStyle.

From the email chain there doesn't seem an easy way to apply any code style configs globally. This is more of PoC as to what I would like to see. How it is implemented/maintained I have no idea.

The settings should apply as soon as you save any changes.

Might be a good idea to do a global cleanup at some point

BeckerFrank commented 1 year ago

I think we can use the following.

For new Java projects we must copy the two files to the new project.

Thoughts?

ruspl-afed commented 1 year ago

+1 for project specific settings. Yes, they require more effort to sync but guarantees that changes done in any Eclipse IDE will use the same settings. I know that some people create linked .pref file that points to a common file, but I have no practical experience with this solution.

@gnl42 it is a bit tricky to discuss the change in this form. Could you please highlight somehow the difference between current and suggested settings? Perhaps you can create a PR for one project to let us see what exactly we try to introduce.

BeckerFrank commented 1 year ago

I add the defaults as a separate files.

Compare of the Formatter with the Eclipse Formatter shows the following changes. <setting id="org.eclipse.jdt.core.formatter.use_tabs_only_for_leading_indentations" value="true"/> <setting id="org.eclipse.jdt.core.formatter.tabulation.char" value="space"/>

Compare Cleanup <setting id="cleanup.array_with_curly" value="true"/> <setting id="cleanup.use_autoboxing" value="true"/> <setting id="cleanup.remove_trailing_whitespaces_ignore_empty" value="true"/> <setting id="cleanup.system_property_file_encoding" value="true"/> <setting id="cleanup.no_super" value="true"/> <setting id="cleanup.stringconcat_to_textblock" value="true"/> <setting id="cleanup.system_property_path_separator" value="true"/> <setting id="cleanup.instanceof" value="true"/> <setting id="cleanup.boolean_literal" value="true"/> <setting id="cleanup.no_string_creation" value="true"/> <setting id="cleanup.use_unboxing" value="true"/> <setting id="cleanup.system_property_line_separator" value="true"/> <setting id="cleanup.map_cloning" value="true"/> <setting id="cleanup.try_with_resource" value="true"/> <setting id="cleanup.use_this_for_non_static_method_access" value="true"/> <setting id="cleanup.use_blocks" value="true"/> <setting id="cleanup.multi_catch" value="true"/> <setting id="cleanup.collection_cloning" value="true"/> <setting id="cleanup.convert_to_enhanced_for_loop_if_loop_var_used" value="false"/> <setting id="cleanup.make_variable_declarations_final" value="true"/> <setting id="cleanup.system_property_boolean" value="true"/> <setting id="cleanup.use_directly_map_method" value="true"/> <setting id="cleanup.system_property_file_separator" value="true"/> <setting id="cleanup.comparing_on_criteria" value="true"/> <setting id="cleanup.stringbuffer_to_stringbuilder" value="true"/> <setting id="cleanup.instanceof_keyword" value="true"/> <setting id="cleanup.remove_trailing_whitespaces_all" value="false"/> <setting id="cleanup.valueof_rather_than_instantiation" value="true"/> <setting id="cleanup.make_parameters_final" value="true"/> <setting id="cleanup.system_property" value="true"/> <setting id="cleanup.convert_to_switch_expressions" value="true"/> <setting id="cleanup.use_this_for_non_static_field_access" value="true"/> <setting id="cleanup.objects_equals" value="true"/> <setting id="cleanup.convert_functional_interfaces" value="true"/> <setting id="cleanup.format_source_code" value="true"/> <setting id="cleanup.else_if" value="true"/> <setting id="cleanup.join" value="true"/> <setting id="cleanup.use_parentheses_in_expressions" value="true"/> <setting id="cleanup.hash" value="true"/> <setting id="cleanup.stringconcat_stringbuffer_stringbuilder" value="true"/> <setting id="cleanup.correct_indentation" value="true"/> <setting id="cleanup.use_var" value="true"/> <setting id="cleanup.correct_indentation" value="true"/> <setting id="cleanup.use_var" value="true"/>

ruspl-afed commented 1 year ago

1.

<setting id="org.eclipse.jdt.core.formatter.use_tabs_only_for_leading_indentations" value="true"/>
<setting id="org.eclipse.jdt.core.formatter.tabulation.char" value="space"/>

Does it mean that we will need to replace Tab with Spaces everywhere? If yes - what is the benefit?

2.

<setting id="cleanup.multi_catch" value="true"/>

Please, don't. People will just stop thinking what are they catching at all. Trivial example is InvocationTargetException and InterruptedException : they need different handling by contract.

3.

<setting id="cleanup.make_parameters_final" value="true"/>
<setting id="cleanup.make_variable_declarations_final" value="true"/>

I'm all for immutability, but final keyword everywhere looks excessive.

4.

<setting id="cleanup.use_var" value="true"/>

I understand, for some cases type for variable declaration looks excessive, but do we want all the code to always use var ?

merks commented 1 year ago

Another concern is that mass modification also tends to destroy the history. For a project where the original folks are gone, understanding the history is relatively important

gnl42 commented 1 year ago

Redid the PR for just one project org.eclipse.mylyn.commons.core

I feel like I'm missing some conversation channel here. I keep finding out changes that I have no idea has been worked on.

ruspl-afed commented 1 year ago

Redid the PR for just one project org.eclipse.mylyn.commons.core

Do you have plans to complete this issue @gnl42 ?

I feel like I'm missing some conversation channel here. I keep finding out changes that I have no idea has been worked on.

All the conversations I know about take place in the issues and PR of this repository.

gnl42 commented 1 year ago

Right now I think I'm waiting for you @ruspl-afed , You asked for a single project setup so I did org.eclipse.mylyn.commons.core

ruspl-afed commented 1 year ago

Sorry, I missed the fact that you're waiting for me. Perhaps it would be better to create a PR to discuss the proposed changes.

BeckerFrank commented 1 year ago

Redid the PR for just one project org.eclipse.mylyn.commons.core

Do you have plans to complete this issue @gnl42 ?

I feel like I'm missing some conversation channel here. I keep finding out changes that I have no idea has been worked on.

All the conversations I know about take place in the issues and PR of this repository.

If it is all right for everyone we can also use Discussions or chat.eclipse.org.

ruspl-afed commented 1 year ago

Do you plan to create a PR from your branch @gnl42 ?

gnl42 commented 1 year ago

Sure.