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.

Sign up for GitHub

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

Jump to bottom

Lombok support v1 #2519

Merged
merged 5 commits into from
Jun 29, 2022
Merged

Lombok support v1 #2519

merged 5 commits into from
Jun 29, 2022

Conversation

XiaoYuhao
Copy link
Contributor

Add the lombok support:

  1. It can automatically detect whether the project has imported the lombok by checking all classpath.
  2. It can show the current version of the lombok in the language status bar.
  3. It can dynamically detect the version change of the lombok.

@testforstephen testforstephen self-requested a review June 23, 2022 14:04
@testforstephen
Copy link
Collaborator

Pls fix tslint errors first. You can run the command npm run tslint locally to check tslint errors.

testforstephen
testforstephen reviewed Jun 24, 2022
View reviewed changes
package.json Outdated Show resolved Hide resolved
@testforstephen testforstephen marked this pull request as ready for review June 26, 2022 15:00
testforstephen
testforstephen requested changes Jun 27, 2022
View reviewed changes
src/lombokSupport.ts Outdated Show resolved Hide resolved
src/lombokSupport.ts Outdated Show resolved Hide resolved
src/lombokSupport.ts Outdated Show resolved Hide resolved
src/lombokSupport.ts Outdated Show resolved Hide resolved
src/lombokSupport.ts Outdated Show resolved Hide resolved
src/lombokSupport.ts Outdated Show resolved Hide resolved
src/lombokSupport.ts Outdated Show resolved Hide resolved
src/lombokSupport.ts Outdated Show resolved Hide resolved
src/lombokSupport.ts Outdated Show resolved Hide resolved
src/lombokSupport.ts Outdated Show resolved Hide resolved
testforstephen
testforstephen requested changes Jun 28, 2022
View reviewed changes
Copy link
Collaborator

@testforstephen testforstephen left a comment

Choose a reason for hiding this comment

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

Play it on a multi-module maven projects, overall it works well. Here is a minor issue.

Toggle on/off the setting java.jdt.ls.lombokSupport.enabled a few times, the language status shows some weird lombok status.
image

src/settings.ts Outdated Show resolved Hide resolved
src/lombokSupport.ts Outdated Show resolved Hide resolved
src/lombokSupport.ts Outdated Show resolved Hide resolved
src/lombokSupport.ts Outdated Show resolved Hide resolved
src/lombokSupport.ts Show resolved Hide resolved
testforstephen
testforstephen approved these changes Jun 29, 2022
View reviewed changes
Copy link
Collaborator

@testforstephen testforstephen left a comment

Choose a reason for hiding this comment

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

LGTM. Tried it on maven and gradle lombok project, it works well. I think it's ready to merge.

In the current version, it prompts the user to reload the window for lombok support. We have tried restarting the language server without reloading, which is technically possible, but we found that this has side effects on third-party extensions, some of which report errors about delegate command exists. So in v1, we still use reloading to restart the language server. We will further investigate how to make third-party extensions work properly with language server restart.

Once the PR is merged, we'll update the wiki description as well. https://github.com/redhat-developer/vscode-java/wiki/Lombok-support

@rgrunber rgrunber requested review from rgrunber and snjeza June 29, 2022 17:10
Yuhao Xiao and others added 5 commits June 29, 2022 13:55
@rgrunber
Add the lombok support:
f2c057f 
1. It can automatically detect whether the project has imported the
   lombok by checking all classpath.
2. It can show the current version of the lombok in the language status
   bar.
3. It can dynamically detect the version change of the lombok.

Signed-off-by: Yuhao Xiao <t-yuhaoxiao@microsoft.com>
@rgrunber
Fix some issues about lombok support.
3b9cc0b 
Signed-off-by: Yuhao Xiao <t-yuhaoxiao@microsoft.com>
@rgrunber
Update some code about lombok support.
f10e937 
Signed-off-by: Yuhao Xiao <t-yuhaoxiao@microsoft.com>
@rgrunber
1. Add 'java.jdt.ls.lombokSupport.enabled' setting description in
1bdc1e7 
   README.md.
2. Update the tooltip of the LombokConfigureCommand.
3. Remove the unnecessary updateActiveLombokPath at initialization.

Signed-off-by: Yuhao Xiao <t-yuhaoxiao@microsoft.com>
@rgrunber
Continue building with Node 14.
3cfbd9d 
- Use lockFileVersion v1 for now as v2 would require moving to Node 16
  in our builds

Signed-off-by: Roland Grunberg <rgrunber@redhat.com>
@rgrunber
Copy link
Member

rgrunber commented Jun 29, 2022 •
edited
Loading

I've rebased this change and just reverted the part about using lockFileVersion 2 since that would require npm v7, and we currently build with Node v14 (npm v6). The addition of htmlparser2 seems to work just fine keeping the lockfile version the same.

@rgrunber rgrunber merged commit 4810edd into redhat-developer:master Jun 29, 2022
@rgrunber rgrunber added this to the End June 2022 milestone Jun 29, 2022
@testforstephen testforstephen mentioned this pull request Jul 12, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snjeza snjeza snjeza approved these changes

@rgrunber rgrunber rgrunber approved these changes

@testforstephen testforstephen testforstephen approved these changes

Assignees
No one assigned
Labels
enhancement Lombok
Projects
None yet
Milestone
End June 2022
Development

Successfully merging this pull request may close these issues.
4 participants
@XiaoYuhao @testforstephen @rgrunber @snjeza