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

Path params quickfix #278

Merged
merged 12 commits into from
Apr 14, 2022
Merged

Conversation

dalidia
Copy link
Contributor

@dalidia dalidia commented Mar 24, 2022

Resolves #276

@dalidia dalidia marked this pull request as ready for review April 5, 2022 17:32
Copy link
Contributor

@giancarlopernudisegura giancarlopernudisegura left a comment

Choose a reason for hiding this comment

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

The quick fix works well on my end.

@@ -31,20 +33,21 @@
import org.eclipse.lsp4jakarta.jdt.codeAction.proposal.ModifyAnnotationProposal;

/**
* Quickfix for adding missing attributes to annotations
* Quickfix for adding missing attributes to new or existing annotations
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we clarify a bit further?
"Quickfix for adding new annotations or adding attributes to existing annotations"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do that

Comment on lines 124 to 130
protected String getLabel(String annotation, String... attributes) {
String type = "";

String name = "Add " + attributes[0] + " to " + annotation;
name = name.concat(type);
return name;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally I think we should handle the two label cases in this class (adding an annotation & adding an attribution to an annotation).
What do you think about this label being "Insert @Annotation" and then we create a new class "InsertAnnotationAttributesQuickFix" which extends "InsertAnnotationQuickFix" and overrides the label to the above? Then we don't have to include the label specific logic in "AddPathParamsQuickFix" and it can easily be reused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds like a good idea! will implement that!

Copy link
Contributor

@kathrynkodama kathrynkodama left a comment

Choose a reason for hiding this comment

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

looks good to me, thanks @dalidia

@kathrynkodama kathrynkodama merged commit a8608d1 into eclipse:main Apr 14, 2022
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.

WebSocket: Add Quickfix for PathParams annotation for certain parameters
4 participants