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

Update PageRequest.java: fix small JavaDoc issue #867

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hohwille
Copy link

just stumbled across a false vs. true in your JavaDoc and thought I simply create a PR.

@hohwille
Copy link
Author

Just saw your contributing guidelines:
https://github.com/jakartaee/data/blob/f7fbd095443c43d9c3a62ed75ac196f405ac8c1d/CONTRIBUTING.adoc#developer-certificate-of-origin---dco

Feel free to close for this small change, probably not worth it. You can also replace the inverted boolean literal yourself...
In case I ever create another PR, I will follow your rules...

@hohwille
Copy link
Author

hohwille commented Oct 10, 2024

p.s.: when an existing thing out in the wild is later considered for standardization things could be challenged more.
JPA is so full of flaws that have been blindly taken over from hibernate.
Jakarta data is on the same way with the "magic query from method name" approach adopted from spring-data.

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}.
Copy link
Contributor

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

@njr-11
Copy link
Contributor

njr-11 commented Oct 11, 2024

@hohwille thanks for your feedback.

Just saw your contributing guidelines:
https://github.com/jakartaee/data/blob/f7fbd095443c43d9c3a62ed75ac196f405ac8c1d/CONTRIBUTING.adoc#developer-certificate-of-origin---dco

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.

Jakarta data is on the same way with the "magic query from method name" approach

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

And having findAll by default is also a huge anti-pattern

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.

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.

3 participants