search

eclipse / lsp4e

Language Server Protocol support in Eclipse IDE
Eclipse Public License 2.0
57 stars 54 forks source link

Minor code cleanup #998

Closed sebthom closed 2 months ago

sebthom commented 2 months ago

This PR fixes a few minor issues detected using static code analysis tools.

sebthom commented 2 months ago

Would it be good to add the analysis tools that you used to our verify jobs?

I know that @mickaelistria is not a big fan of adding more "compliance" tools to the build process as from his view it prevents people from contributing. I think there are probably some checks worthy to be checked automatically, esp. that influence security, stability and performance. I personally would also like to have a check that ensures the source code is formatted using the configured Eclipse formatter profile as it is super annoying that currently each file is a unicorn and when I use the code formatter dozens of lines not related to my code changes get modified.

Which ones did you use?

I used strict eclipse compiler settings, sonarlint and ucdetector. in other projects I also use checkstyle.

Also, I think you have converted some classes to lambdas. We could adapt the configuration of the JDT for our plugins, so that the editor does that on save. Would it be good to do it?

Before introducing automatic save actions we would need to run them once across the whole code base in a single commit as otherwise every time you edit a file code unintentionally may changed that is not related to what you are currently modifying.

So we should have an Eclipse cleanup profile that is in sync with enabled Save Actions. This would be handy if implemented: https://bugs.eclipse.org/bugs/show_bug.cgi?id=178429

mickaelistria commented 2 months ago

I know that @mickaelistria is not a big fan of adding more "compliance" tools to the build process as from his view it prevents people from contributing

I'm not against tools/configuration adding recommendation in the workspace or in some build report, I just want to avoid that a contribution gets delayed or even abandoned by its contributor because of recommendations becoming constraints during review and then putting to bar often too high for newcomers. But configuring the project settings to have automated refactorings (eg convert to lambda), more warnings and so on is IMO totally fine; as they can be very quickly and easily considered while developing; and that we still allow new code in, even if it introduces some warnings (as long as reviewer has evaluated they are not causing bugs)..

rubenporras commented 2 months ago

Before introducing automatic save actions we would need to run them once across the whole code base in a single commit as otherwise every time you edit a file code unintentionally may changed that is not related to what you are currently modifying.

That might not be needed. The project is not so big and usually the refactors are easy to recognize, so adapting the files next time they are touched would be fine for me (of course cleaning up is also fine).

In any case , we do not need to it know, it is just that since where doing some of these refactors that the JDT can do, I wondered if you would like to just add the preference (and eventually run the cleanup).