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

Minor code cleanup #998

Merged
merged 14 commits into from
May 15, 2024
Merged

Minor code cleanup #998

merged 14 commits into from
May 15, 2024

Conversation

sebthom
Copy link
Member

@sebthom sebthom commented May 15, 2024

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

Copy link
Contributor

@rubenporras rubenporras left a comment

Choose a reason for hiding this comment

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

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

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?

If you think so, I could lookup how to do that.

*/
protected abstract SelectionRangeHandler.Direction getDirection();
}
/*******************************************************************************
Copy link
Contributor

Choose a reason for hiding this comment

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

What did change in this file? Line endings?

Copy link
Member Author

Choose a reason for hiding this comment

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

In this class an anonymous inner class was replaced by a shorter lambda, see 3075595

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, looks like the line endings changed too as part of 6f33fc4. So it probably was with Windows line endings committed to git before. To avoid this, we have a .gitattributes file in tm4e https://github.com/eclipse/tm4e/blob/main/.gitattributes

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation.
We could have it also for LSP4E, I do not have a strong opinion for or against it.
@mickaelistria , do you have an opinion?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can prepare a PR where I normalize the line endings of all files in the repo and add the gitattributes file, similar to how I did it for tm4e.

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds great to me.

@sebthom
Copy link
Member Author

sebthom commented May 15, 2024

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
Copy link
Contributor

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
Copy link
Contributor

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).

@sebthom sebthom merged commit d9061f8 into eclipse-lsp4e:master May 15, 2024
2 checks passed
@sebthom sebthom deleted the refact branch May 15, 2024 12:18
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