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

Improve documentation of Caching annotations #28183

Closed
wants to merge 2 commits into from

Conversation

luvarqpp
Copy link
Contributor

There was missing basic information for conditional attribute of Cacheable annotation. Perhaps something similar should be added also to unless attribute.

There was missing basic information for `conditional` attribute of `Cacheable` annotation. Perhaps something similar should be added also to `unless` attribute.
Copy link
Member

@snicoll snicoll left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Making it obvious that the expression should return a Boolean and what the value should be to cache the item is a nice improvement to the Javadoc.

I've made a comment of moving this as an opening statement, rather than at the end. Would you be willing to upgrade your PR in that direction?

@@ -137,6 +137,7 @@
* can be accessed via {@code #root.args[1]}, {@code #p1} or {@code #a1}. Arguments
* can also be accessed by name if that information is available.</li>
* </ul>
* If condition is evaluated to {@code true}, result is cached.
Copy link
Member

Choose a reason for hiding this comment

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

I think this would be better as an opening statements, rather than at the end. We should also update the paragraph about the default to mention that if no expression is set, the method result is always cached.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice words for me, thanks. I am on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it better now?
Should I squash / rebase?

@snicoll snicoll added the type: documentation A documentation task label Mar 15, 2022
@snicoll snicoll added this to the 5.3.x milestone Mar 15, 2022
@snicoll snicoll self-assigned this Jul 29, 2022
@snicoll snicoll modified the milestones: 5.3.x, 5.3.23 Jul 29, 2022
@snicoll snicoll changed the title Clarify documentation of Cacheable Improve documentation of Caching annotations Jul 29, 2022
snicoll pushed a commit that referenced this pull request Jul 29, 2022
snicoll added a commit that referenced this pull request Jul 29, 2022
Apply the same improvements to CacheEvict and CachePut.

See gh-28183
@snicoll snicoll closed this in 5ab55df Jul 29, 2022
@snicoll
Copy link
Member

snicoll commented Jul 29, 2022

@luvarqpp thank you for making your first contribution to Spring Framework.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: documentation A documentation task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants