-
Notifications
You must be signed in to change notification settings - Fork 31
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
Conversation
Signed-off-by: Otavio Santana <[email protected]>
Signed-off-by: Otavio Santana <[email protected]>
Signed-off-by: Otavio Santana <[email protected]>
@njr-11 don't forget of review this one. |
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.
* | ||
* @return a request for the next page. | ||
*/ | ||
PageRequest<T> nextPageRequest(); | ||
Optional<PageRequest<T>> nextPageRequest(); |
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.
@njr-11 it still require this validation.
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.
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
@otaviojava can we close this? With other changes that have gone in, it is no longer possible to get a null from |
Changes
Once we are using optional when the return might be null, such as, it would be great to keep the consistency here as well.