Skip to content
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

Code style improvements #1568

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nettozahler
Copy link
Contributor

What it does

Code style improvements. They should not change the runtime behavior.

How to test

Run existing test suite for the affected class. It should not fail.

Author checklist

@jjohnstn
Copy link
Contributor

jjohnstn commented Aug 7, 2024

Hello @nettozahler Thank you for your interest in Eclipse. While your change is good, we generally avoid just reformatting non-trivial source files. We often do formatting while fixing something else in a file or in a special case on an M1 milestone (we are currently working on M3) to test our own clean-ups on real code or if there is now a new measurably better way in Java to do something. The reason is that if we just do a mass reformatting on a file, the use of git revision information in the editor to trace various pieces of logic back to the git change that introduced them is then of no use because it is overridden by the formatting changes which are the last changes made. This makes it more difficult to fix things and determine what logic is doing because often the documentation is found in the commit message or in the corresponding bug/issue being fixed.

@nettozahler
Copy link
Contributor Author

Hi @jjohnstn, thanks for your feedback to this PR. I have created it primarily to learn how to contribute to Github and Eclipse using the "Fork and pull model". So it would be OK for me if the PR is not merged.

OTH I'd like to point out that the PR was not created by some kind of "reformatting tool" but it is hand-crafted. :-) It uses "Pattern Matching for instanceof", simplified if/else and inserts missing Javadoc.

Finally your answer raises the question: how can we modernize the Eclipse source code if we generally avoid multiple improvements like in this PR? I mean contributing to Eclipse is much more fun if the source code does not look like from the Java stone age :-)

@jjohnstn
Copy link
Contributor

jjohnstn commented Aug 8, 2024

Hi @nettozahler It is a balancing act between stability, maintainability, code styling, and modernizing the code base. There has been lots of heated discussion between JDT developers on this topic. The compromise that has been reached is that mass refactoring or reformatting always happens on an M1 milestone so that there is plenty of chance for end-users and developers to test if anything bad happened and to deal with bugs. It has always been that a single clean-up or change is applied en-masse to one or more of the jdt sub-projects. This has happened in the past (e.g. switching to use Text blocks or use try-with-resources). Switching to use pattern matching instanceof is a clean-up we offer. An M1 patch could be offered to do this throughout the jdt.ui repo, but it would just be the one clean-up and no additional code formatting. Beyond that, if you had a bug fix for the file you worked on, you could probably sneak in a bunch of code improvements at the same time and I would probably approve the review. :)

@jjohnstn jjohnstn force-pushed the code-style-improvements branch from c045ce7 to 17a6742 Compare September 10, 2024 19:20
Copy link
Contributor

@jukzi jukzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR contains good improvements like using instanceof, but also adds javadoc that adds no real value. I would expect if you like to have such improvement merged to be only of advantage and that if manually improving only very limited code that the code should be near to perfect at the end. For example it could also use for-each loops, remove silly existing inheritDoc javadoc.
I suggest to close this particular PR without merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants