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

Support separate "Generate Getters" and "Generate Setters" #2086

Merged

Conversation

CsCherrYY
Copy link
Contributor

@CsCherrYY CsCherrYY commented May 12, 2022

related to redhat-developer/vscode-java#2450

Signed-off-by: Shi Chen chenshi@microsoft.com

@@ -92,4 +93,9 @@ public static class GenerateAccessorsParams {
CodeActionParams context;
AccessorField[] accessors;
}

public static class ResolveUnimplementedAccessorsParams {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rename it to AccessorCodeActionParams, and it can keep better compatible if you make AccessorCodeActionParams to extend CodeActionParams. Its default AccessorKind can be BOTH.

Copy link
Contributor Author

@CsCherrYY CsCherrYY May 19, 2022

Choose a reason for hiding this comment

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

Addressed in cf595f6

@snjeza
Copy link
Contributor

snjeza commented May 18, 2022

test this please

Copy link
Contributor

@rgrunber rgrunber left a comment

Choose a reason for hiding this comment

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

Change looks good overall. Seems to be working well for me.

} else {
Command command = new Command(ActionMessages.GenerateGetterSetterAction_ellipsisLabel, COMMAND_ID_ACTION_GENERATEACCESSORSPROMPT, Collections.singletonList(params));
Command command = new Command(ellipsisActionMessage, commandId, Collections.singletonList(params));
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wondering : If instead of passing params: CodeActionParams you passed a ResolveUnimplementedAccessorsParams, would that help eliminate the 3 separate registered commands ( https://github.com/redhat-developer/vscode-java/pull/2450/files#diff-ca92d1afe836a483259dfc56e55ada3e33076f195fd4f2e7a0fd06be4bfa664f ) on the vscode-java side ? Is it worth doing this ? We pass the kind from client to server via ResolveUnimplementedAccessorsParams to avoid having 3 versions of java/resolveUnimplementedAccessors for each case.

If it's not worth it, this approach is also fine.

Copy link
Contributor Author

@CsCherrYY CsCherrYY May 19, 2022

Choose a reason for hiding this comment

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

I think it could make our implementation clearer and it's worth doing. I have done it via the new commit cf595f6

Signed-off-by: Shi Chen <chenshi@microsoft.com>
Signed-off-by: Shi Chen <chenshi@microsoft.com>
Copy link
Contributor

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

Works for me.

@testforstephen testforstephen added this to the Early June 2022 milestone May 23, 2022
@testforstephen testforstephen merged commit 54a0253 into eclipse-jdtls:master May 23, 2022
@CsCherrYY CsCherrYY deleted the cs-getter-setter-saparate branch June 30, 2022 00:46
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.

None yet

4 participants