-
Notifications
You must be signed in to change notification settings - Fork 30
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
Update PageRequest.java: fix small JavaDoc issue #867
base: main
Are you sure you want to change the base?
Conversation
Just saw your contributing guidelines: Feel free to close for this small change, probably not worth it. You can also replace the inverted boolean literal yourself... |
p.s.: when an existing thing out in the wild is later considered for standardization things could be challenged more. And having findAll by default is also a huge anti-pattern:
In real life projects this often leads to catastrophic disasters when newbie developers find this method and start using it without thinking what may happen if you have millions of records. All just my very personal opinion - feel free to ignore ;) |
@@ -31,7 +31,7 @@ | |||
* <p>A query method of a repository may have a parameter of type | |||
* {@code PageRequest} if its return type indicates that it may return | |||
* multiple entities, that is, if its return type is an array type, | |||
* {@code List}, {@code Stream}, {@link Page}, or {@link CursoredPage}. | |||
* {@link List}, {@code Stream}, {@link Page}, or {@link CursoredPage}. |
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.
Maybe moving all of those tags to link
@hohwille thanks for your feedback.
This is standard process for all Jakarta EE projects under Eclipse. If you are able to, I'd encourage you to sign the Eclipse Contributor Agreement so that we can merge your PR.
The Query by Method Name does offer some compatibility with existing solutions, but looking forward to the next version, we are planning some alternative patterns (such as #857 and #829).
I remember there was a review of BasicRepository methods prior to version 1.0, due to which several were omitted. I don't recall why findAll made it through. While it could be useful in some specific scenarios, in general, it is as you said, and anti-pattern. I would have rather seen users opt in to it than get it by default. |
just stumbled across a
false
vs.true
in your JavaDoc and thought I simply create a PR.