-
Notifications
You must be signed in to change notification settings - Fork 803
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
Added support for slicing multiple times in Search class #1771
Conversation
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.
Thanks! The tests make sense, but the code is difficult to read for me, so I asked clarification questions.
@pquentin I have simplified the slicing even more. If the from or the size aren't specified in the slicing, then they are not included in the final request body. So for example:
|
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.
Thanks! The logic looks good. It just needs documentation.
My only remaining worry is that this is a breaking change. Would it be useful to warn about using multiple slices in 8.13.1 and 8.14 and then switch to the new behavior in 8.15? Some users do pay attention to our warnings: elastic/elasticsearch-py#2518 (comment).
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.
Thanks! LGTM. We'll have to make sure to label this as a breaking change in the release notes.
0074785
to
863f033
Compare
* Added support for slicing multiple times in Search class Fixes #799 * simplify slicing logic * A few more slicing unit tests * Removed unnecessary comment * simplified slicing logic even more * added suggested changes * add more slicing examples to documentation (cherry picked from commit fb57759)
* Added support for slicing multiple times in Search class Fixes #799 * simplify slicing logic * A few more slicing unit tests * Removed unnecessary comment * simplified slicing logic even more * added suggested changes * add more slicing examples to documentation (cherry picked from commit fb57759) Co-authored-by: Miguel Grinberg <[email protected]>
Fixes #799
This PR implements Pythonic slicing for the
Search
object.Note: due to introducing backward incompatibilities, this change is scheduled to be incorporated in release 8.15. Starting with release 8.13.1 a warning will alert developers slicing results multiple times that this breaking change is coming.