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

Add support for @ConditionalOnResource annotation attributes #1339

Closed
wants to merge 3 commits into from
Closed

Conversation

ksankaranara-vmw
Copy link
Collaborator

@martinlippert
Copy link
Member

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.

@ksankaranara-vmw
Copy link
Collaborator Author

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

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.

2 participants