mass clean up and bring back default config

[Bananeweizen]

This issue is mainly to explain why many other things should be done first before bringing back the default checkstyle configuration. To recap, there are around 7000 issues with the default config. Fixing those should be as efficient as possible, and should not lead to contradictions with how the Eclipse IDE works where the eclipse-cs source is developed (because otherwise we may play ping-pong between Eclipse formatting something, Checkstyle complaining, manual fixing, and repeating the cycle all over).

• Merge as many open PRs as possible. They would be in git conflicting state with all the reformatting changes coming later.
• Enable automatic import organizing. Modify settings to match Checkstyle rules. Run organize imports on all sources.
• Search/replace all the $NON-NLS markers in source code. Those are needed only for plugins which use bundle localization, which isn't the case for eclipse-cs.
• Enable auto-format on save in Eclipse. Right now the formatter is not active at all, therefore newly written code doesn't fit the Checkstyle settings (like line length). Modify formatter settings to fit Checkstyle settings. Format all existing sources using the Eclipse formatter.
• Enable cleanup on save in Eclipse. Enable as many options as possible. Run the cleanups. (Those cleanups work like automated refactorings. E.g. foo.equals("const") will automatically be reordered, thereby automatically fixing the Checkstyle issue.)
• ... maybe more coming up along the way
• enable standard config again, hoping the number should have gone down drastically

[rnveach]

Enable automatic import organizing
Enable auto-format on save in Eclipse
Enable cleanup on save in Eclipse

I just wanted to point out that while I do enable these for my own projects, these will only work if everyone is using Eclipse as their IDE (and sometimes the same version of Eclipse). You can't expect all contributors to do this as I know for a fact some don't use Eclipse. This will cause extra differences in PRs between users who do use Eclipse and who don't. Some of the odd formatting you were talking about, may have been from someone not using Eclipse.

I would recommend to find some way for this also to be run in CI if you plan to enable this for Eclipse users to unite both sets of users. See how main repo does some of this in:
https://github.com/checkstyle/checkstyle/blob/master/config/org.eclipse.jdt.core.prefs
https://github.com/checkstyle/checkstyle/blob/master/.ci/eclipse-compiler-javac.sh#L54

[romani]

usage of Eclipse auto formatting is fine, but it should be in non-conflicting with checkstyle rules.

[Bananeweizen]

I just wanted to point out that while I do enable these for my own projects, these will only work if everyone is using Eclipse as their IDE

Actually, I expect every contributor to the eclipse-cs Java source code to use Eclipse, otherwise changes are basically not testable. But that is not even the point here. Right now no formatting happens when using Eclipse, since the formatter is disabled in the project properties, which is why everything gets stored as typed (which is the worst of all formatting rules in shared source code). After enabling it and configuring it to use the same values like the Checkstyle rules mandate, at least everyone using Eclipse will no longer violate any formatting related rules. Nobody should ever reformat code manually to fulfill Checkstyle rules. Such work should be done by some automation in the IDE, and the obvious automation for that in the Eclipse eco system are formatter, cleanup actions etc. None of the things I intend to change will affect users of other IDEs, as long as those other IDEs already produce Checkstyle compliant code.

Tycho automatically uses the *.prefs of the project, therefore it's normally not necessary to point to an artificial config file during compilation. To be able to share a single config, the best way is to use "linked resources" for the prefs files. At work I also use solutions that copy all prefs from a single project into all others as real files, but the linked resources will probably fit best here.

Side note: In the linked script ecj is downloaded via wget from some eclipse mirror. You can also get just ecj via Maven: https://central.sonatype.dev/artifact/org.eclipse.jdt/ecj/3.31.0/overview. Not sure if that might help simplifying that script.

(and sometimes the same version of Eclipse).

That's not exactly correct. Obviously, if you use an old version of Eclipse, it cannot know newly added preferences in the preference settings. But it does work for all preferences that this old Eclipse version knows. So it basically only ignores the (from its point of view) future preferences. To avoid this, whenever you upgrade your Eclipse IDE to a new release, open the project properties of a project in your workspace, iterate over all pages in the project properties and then close the dialog using OK, even though you didn't change any values. That will add all the new keys and default values into all *.prefs files, which you can then easily review before committing them.

usage of Eclipse auto formatting is fine, but it should be in non-conflicting with checkstyle rules.

That's exactly the target of all the above. Right now developing in Eclipse leads to violations (unless you manually tweak whitespace and similar things in the editor), afterwards not anymore.

[romani]

Nobody should ever reformat code manually to fulfill Checkstyle rules

Checkstyle is not only about formatting, but yes we have bunch of spacing related Checks.
But we need to find not conflicting settings and use checkstyle as source of truth. Ide follows formatting set of Checks.

Eclipse preference will work stable only all use same version of Eclipse. It is impossible to enforce on contributors, so we need CI to be a side that tell all how code should looks like and what rules to follow. For now in CI only checkstyle can do this.

I hope there is already headless way to execute eclipse formatting over code, and we can change a strategy. But for now we follow checkstyle on this to keep code on leash from outside contributors.

In a past we did bunch of attempts to make eclipse configs that matching Checks, but all that failed with each Eclipse new version, as some behavior is changing all the time, even properties are same.

[Bananeweizen]

Just so you know this is not forgotten. I have a WIP change locally which does configure formatter, cleanup, imports etc. and by applying all of those >90% of all checkstyle issues get fixed. I'm still finetuning...

[romani]

Please keep sending PRs with partial updates, reviewing huge PR is not a fun

[Bananeweizen]

That is a single change. It applies the formatter and cleanups as automated refactorings to every file in the project. There is no manual code editing involved, but still every file changes. Same as if you run some beautifier/formatter on the command line. There will be additional changes afterwards for e.g. renaming bad identifiers. Those will be as small as possible, but that very first one is huge, and reviewing it is pointless, unless you don't trust the automations. And since it changes every file, I will also not create it at a time like now with lots of unmerged changes, since all of them would be in merge conflict afterwards.

[romani]

@Calixte , there are about 4 PRs assigned to you for review , please find time to review them or give us some confirmation that you ok to merge without your review.

[anonymousdouble]

Hello, @Bananeweizen @romani , Interesting!

I found it intriguing that Eclipse-cs now uses Checkstyle to format its code. I noticed the configuration files, specifically the [checkstyle_checks.xml ] (https://github.com/checkstyle/eclipse-cs/blob/master/config/checkstyle_checks.xml) in the Eclipse-cs repository. I'm curious, do developers of Eclipse-cs manually set these properties, meticulously confirming each rule?

BTW, I'm wondering if Eclipse-cs has established coding standards to help developers write configurations of checkstyle_checks.xml to make it conform to coding rules of Eclipse-cs.