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 API returning Optional instead of Null #497

Closed
wants to merge 3 commits into from

Conversation

otaviojava
Copy link
Contributor

Changes

  • For consistency, I've updated the return to be Optional instead of null.

Once we are using optional when the return might be null, such as, it would be great to keep the consistency here as well.

@otaviojava otaviojava requested a review from njr-11 February 28, 2024 10:47
@otaviojava otaviojava mentioned this pull request Feb 28, 2024
@otaviojava
Copy link
Contributor Author

@njr-11 don't forget of review this one.

Copy link
Contributor

@njr-11 njr-11 left a comment

Choose a reason for hiding this comment

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

@njr-11 don't forget of review this one.

Let's wait on reviewing this one because #506 has a different solution that will make this pull unnecessary because it will no longer be possible to receive a null.

*
* @return a request for the next page.
*/
PageRequest<T> nextPageRequest();
Optional<PageRequest<T>> nextPageRequest();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@njr-11 it still require this validation.

Copy link
Contributor

@njr-11 njr-11 Mar 1, 2024

Choose a reason for hiding this comment

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

That's only because the other pull missed the part about raising an exception from nextPageRequest/previousPageRequest instead of returning null. I can make sure that gets included when updating the TCK to cover hasNext/hasPrevious. Then this will follow the pattern of java.util.Iterator and java.util.ListIterator where you must check hasNext (or hasPrevious) before requesting the next/previous

@njr-11
Copy link
Contributor

njr-11 commented Mar 8, 2024

@otaviojava can we close this? With other changes that have gone in, it is no longer possible to get a null from nextPageRequest.

@otaviojava otaviojava closed this Mar 9, 2024
@otaviojava otaviojava deleted the update-optional-api branch March 9, 2024 09:16
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