-
Notifications
You must be signed in to change notification settings - Fork 206
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
GH-1323, GH-1324: Cron Expressions completion proposals and inlay hints #1357
GH-1323, GH-1324: Cron Expressions completion proposals and inlay hints #1357
Conversation
…letion proposals and inlay hints
|
||
Set<String> alreadyMentionedValues = alreadyMentionedValues(node); | ||
|
||
AnnotationAttributeCompletionProvider completionProvider = this.completionProviders.get(attributeName); | ||
if (completionProvider != null) { | ||
List<String> candidates = completionProvider.getCompletionCandidates(project); | ||
|
||
List<String> filteredCandidates = candidates.stream() | ||
if (completionProvider instanceof CronExpressionCompletionProvider) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... this bit does not look pretty... Some Cron completions specific bit in the generic AnnotationAttributeCompletionProcessor
... Any chance we can push this logic down to the CRON completion provider somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest that we just switch the method AnnotationAttributeCompletionProvider.getCompletionCandidates
to return labels for the proposals as well and don't add an additional method for that. The AnnotationAttributeCompletionProcessor
would then not need to distinguish between provider with and without labels and assume instead that the labels are coming from the providers.
Then, we need to adapt all the various provider implementations to return labels as well, but that should't be a big deal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vudayani Do you want to do those changes in the PR directly or do you want me to do those changes in main first, and then you adapt the PR to that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@martinlippert I can do the changes in this PR if it can wait until tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, sounds good. Looking forward to the updated PR then.
Looks great, thanks so much for the updated PR. Merged now. |
No description provided.