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

Remove Sort at PageRequest #639

Merged
merged 20 commits into from
Apr 4, 2024
Merged

Remove Sort at PageRequest #639

merged 20 commits into from
Apr 4, 2024

Conversation

otaviojava
Copy link
Contributor

@otaviojava otaviojava commented Apr 4, 2024

Changes

The goal is to define single responsibility at PageRequest

⚠️ I saw the PRs around specification and TCK, thus, if you don't mind. I would break it into small pieces; thus, once it is merged, I will work on the TCK and the spec.

Signed-off-by: Otavio Santana <otaviopolianasantana@gmail.com>
Signed-off-by: Otavio Santana <otaviopolianasantana@gmail.com>
Signed-off-by: Otavio Santana <otaviopolianasantana@gmail.com>
Signed-off-by: Otavio Santana <otaviopolianasantana@gmail.com>
Signed-off-by: Otavio Santana <otaviopolianasantana@gmail.com>
Signed-off-by: Otavio Santana <otaviopolianasantana@gmail.com>
Signed-off-by: Otavio Santana <otaviopolianasantana@gmail.com>
Signed-off-by: Otavio Santana <otaviopolianasantana@gmail.com>
Signed-off-by: Otavio Santana <otaviopolianasantana@gmail.com>
Signed-off-by: Otavio Santana <otaviopolianasantana@gmail.com>
Signed-off-by: Otavio Santana <otaviopolianasantana@gmail.com>
Signed-off-by: Otavio Santana <otaviopolianasantana@gmail.com>
Signed-off-by: Otavio Santana <otaviopolianasantana@gmail.com>
Signed-off-by: Otavio Santana <otaviopolianasantana@gmail.com>
Signed-off-by: Otavio Santana <otaviopolianasantana@gmail.com>
Signed-off-by: Otavio Santana <otaviopolianasantana@gmail.com>
@otaviojava otaviojava marked this pull request as ready for review April 4, 2024 10:31
api/src/main/java/jakarta/data/repository/OrderBy.java Outdated Show resolved Hide resolved
api/src/main/java/jakarta/data/repository/Query.java Outdated Show resolved Hide resolved
api/src/main/java/module-info.java Outdated Show resolved Hide resolved
api/src/main/java/module-info.java Outdated Show resolved Hide resolved
otaviojava and others added 4 commits April 4, 2024 15:43
Co-authored-by: Gavin King <gavin@hibernate.org>
Co-authored-by: Gavin King <gavin@hibernate.org>
Co-authored-by: Gavin King <gavin@hibernate.org>
Signed-off-by: Otavio Santana <otaviopolianasantana@gmail.com>
@otaviojava
Copy link
Contributor Author

@gavinking @njr-11

Copy link
Contributor

@gavinking gavinking left a comment

Choose a reason for hiding this comment

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

I say go ahead and merge it, though I think we should all review all the related javadoc again after the merge.

@gavinking
Copy link
Contributor

I think we should all review all the related javadoc again after the merge.

(Just because I think it's really hard to catch everything.)

@njr-11
Copy link
Contributor

njr-11 commented Apr 4, 2024

I say go ahead and merge it, though I think we should all review all the related javadoc again after the merge.

I had only just started looking at it and already noticed missed places in JavaDoc/examples

@otaviojava
Copy link
Contributor Author

@njr-11 @gavinking merged, I will work to fix the TCK.

@otaviojava otaviojava merged commit 8469847 into main Apr 4, 2024
1 of 3 checks passed
@otaviojava otaviojava deleted the split-page-request branch April 4, 2024 15:30
@KyleAure
Copy link
Contributor

KyleAure commented Apr 4, 2024

Uff. I don't think this should have been merged with compile time errors. Now nothing can be developed or delivered until this is resolved. @otaviojava @gavinking
https://github.com/jakartaee/data/actions/runs/8556638994/job/23446947613

@njr-11
Copy link
Contributor

njr-11 commented Apr 4, 2024

Uff. I don't think this should have been merged with compile time errors.

I totally agree. I tried making a comment that I had only even just started reviewing it and was seeing issues. If it weren't so late in the cycle I'd say we should revert it, but that would only cost more time.

I see that Gavin already fixed one of the compile errors in the API. I'll look into fixing the others in the TCK

@gavinking
Copy link
Contributor

Yeah I messed up when I said "go ahead and merge it", I didn't realize there were compilation errors.

@otaviojava
Copy link
Contributor Author

Uff. I don't think this should have been merged with compile time errors. Now nothing can be developed or delivered until this is resolved. @otaviojava @gavinking https://github.com/jakartaee/data/actions/runs/8556638994/job/23446947613

It was my mistake, sorry everyone.

@otaviojava
Copy link
Contributor Author

otaviojava commented Apr 4, 2024

I am working to fix it now.

To be honest, I expected it to be faster; I did not realize that the IDE does not work properly here.

@otaviojava
Copy link
Contributor Author

Uff. I don't think this should have been merged with compile time errors.

I totally agree. I tried making a comment that I had only even just started reviewing it and was seeing issues. If it weren't so late in the cycle I'd say we should revert it, but that would only cost more time.

I see that Gavin already fixed one of the compile errors in the API. I'll look into fixing the others in the TCK

ops, I saw that you will work on the tck.
I will hold it.

njr-11 added a commit to njr-11/data that referenced this pull request Apr 4, 2024
…ee#639

Signed-off-by: Nathan Rauh <nathan.rauh@us.ibm.com>
njr-11 added a commit to njr-11/data that referenced this pull request Apr 4, 2024
njr-11 added a commit to njr-11/data that referenced this pull request Apr 4, 2024
mswatosh added a commit that referenced this pull request Apr 4, 2024
njr-11 added a commit to njr-11/data that referenced this pull request Apr 4, 2024
Signed-off-by: Nathan Rauh <nathan.rauh@us.ibm.com>
njr-11 added a commit to njr-11/data that referenced this pull request Apr 4, 2024
Signed-off-by: Nathan Rauh <nathan.rauh@us.ibm.com>
otaviojava added a commit that referenced this pull request Apr 5, 2024
specification document is out of sync with #639
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.

None yet

4 participants