sts4 icon indicating copy to clipboard operation
sts4 copied to clipboard

Add support for @ConditionalOnResource annotation attributes

Open ksankaranara-vmw opened this issue 1 year ago • 1 comments

  • Content Assist Support for @ConditionalOnResource annotations
  • Goto Definition Support for @ConditionalOnResource annotations
  • Issue: #1310

ksankaranara-vmw avatar Aug 29 '24 13:08 ksankaranara-vmw

Hey @ksankaranara-vmw, thanks a lot for submitting the PR for this, much appreciated. I have a few suggestions:

  • the PR seems to include a commit and a merge commit from an older contribution. It would be better if you could submit a clean PR that contains the contributions for this issue only
  • I personally like commit messages that reference the corresponding issue (GitHub creates a nice link for that), so having a commit message that starts with GH-1310: <and you real message> would be awesome
  • new files need a copyright statement with 2024 only, some files (maybe copied) mention 2017, 2024, which would mean that the file was created in 2017 and last changed in 2024. Just 2024 is enough here.
  • I am wondering whether you could implement the ConditionalOnResourceProcessor based on a AnnotationAttributeCompletionProvider (e.g. like DependsOnCompletionProcessor does), which would reduce the overall code to produce the proposals instead of having all the code to extract stuff from the AST repeated here. The same maybe applies to the ContextConfigurationProcessor (which should probably renamed to ContextConfigurationCompletionProcessor)

In case you would like to make the suggested changes to the ContextConfigurationProcessor as well, I would suggest a separate PR for that.

Would be awesome if you could take a look. Hope you find my suggestions helpful.

martinlippert avatar Aug 29 '24 19:08 martinlippert

@martinlippert Thanks for taking the time to review this and I totally agree with your suggestions. And sure, will make the changes accordingly and submit two new PRs - one for ConditionalOnResource and one for ContextConfiguration :-) I am closing this PR.

ksankaranara-vmw avatar Aug 30 '24 02:08 ksankaranara-vmw