search

eclipse-lsp4j / lsp4j

A Java implementation of the language server protocol intended to be consumed by tools and language servers implemented in Java.
https://eclipse.org/lsp4j
Other
581 stars 141 forks source link

Refactoring/code cleanup #817

Closed sebthom closed 4 months ago

sebthom commented 4 months ago

To work on #816 I imported the lsp4j project into my Eclipse workspace and was greeted with a lot of compiler and style warnings. With this PR I am addressing a few of the issues. Each type of change is encapsulated in a separate commit for easier review.

If you disagree with some changes I can remove the respective commits.

jonahgraham commented 4 months ago

I'm fine with this - but was confused because I didn't see these warnings, until I realized that the projects were defaulting to workspace settings and my workspace and yours must be different.

Can you please consider adding a commit where you save the compiler warnings/etc and code style setting to the project?

Without your change, in my workspace the only warnings I see are deprecated warnings.

pisv commented 4 months ago

Without your change, in my workspace the only warnings I see are deprecated warnings.

Same here.

BTW, there is also @SuppressWarnings("all") on the interface Tuple, which seems completely unnecessary and has been suppressing the warnings about unused imports in that file.

sebthom commented 4 months ago

@pisv

I'm not a big fan of the last commit that introduces vars, but that's just my personal opinion

I am aware that this is a sensitive topic :-) On the one hand I like code that is explicit on the other hand I like code that is terse. To get the best of both worlds I therefore only use the var keyword when the actual type is still visible on the right side of the assignment either via an explicit cast var foo = (SomeType) obj or if it is a constructor call var foo = new SomeType(). In those cases the providing the type on the left side is really redundant. However I am not a fan of relying on type interference in other cases like var foo = someService.someMethod().get(2).

pisv commented 4 months ago

@sebthom Regarding introducing vars into the code base, your approach certainly makes sense to me, thanks for the detailed description.

It is just that I have grown very much accustomed to always seeing the type on the left side of the variable declaration in the good old Java days :-) So, it is just my personal preference, and I understand that I'm quite biased towards it. Besides, I'm currently spending much more time with TypeScript, and slowly getting the type inference thing :-)

Therefore, I'd like to leave this decision to others. Since @jonahgraham as the project lead apparently has no objections, I think we are good to go with it 👍

jonahgraham commented 4 months ago

Since @jonahgraham as the project lead apparently has no objections

No objections :-)

FWIW your journey sounds just like mine re vars and living in typescript land more and more.

sebthom commented 4 months ago

Can you please consider adding a commit where you save the compiler warnings/etc and code style setting to the project?

@jonahgraham I added my Eclipse compiler settings to the build.gradle file. If you regenerate the eclipse settings using gradle eclipse all projects will be adjusted accordingly. Feel free to change the settings to your liking.

pisv commented 4 months ago

FWIW your journey sounds just like mine re vars and living in typescript land more and more.

After having lived in the Eclipse IDE land for almost 20 years, I started contributing to Eclipse Theia about a year ago :-)