-
Notifications
You must be signed in to change notification settings - Fork 76
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
add support for sort in collection.find function #359
add support for sort in collection.find function #359
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.
I like the idea!
Left some remarks, but nothing major.
arango/utils.py
Outdated
:type sort: Sequence[Json] | ||
:return: Validation success. | ||
:rtype: bool | ||
:raise arango.exceptions.DocumentGetError: If sort parameters are invalid. |
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.
The docstring says DocumentGetError
, but the function raises DocumentParseError
. I think it's better to create a new error type, inheriting ArangoClientError
in exceptions.py
, use that and fix the docstring (for example, SortValidationError
).
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.
Done. I created a new section (Parameter Validation Exceptions) under exceptions.py, which can come handy for other types of validation errors (filter etc.) in the future.
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!
Please update the docstring of this function using the newly created SortValidationError
, along with the two raise
statements at lines 143 and 147.
So I see one of the tests (tests/test_cursor.py::test_cursor_from_execute_query[db0]) is failing for some python versions. Not sure if this is pertinent to my change but for the first commit they were not failing. Same test is passing on my local env (python 3.12.4). Any idea? |
Don't worry about these failing tests, they are unrelated. I'll fix them in #360 |
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.
Only validate_sort_parameters
remains to be fixed, with regards to raising exceptions.
arango/utils.py
Outdated
:type sort: Sequence[Json] | ||
:return: Validation success. | ||
:rtype: bool | ||
:raise arango.exceptions.DocumentGetError: If sort parameters are invalid. |
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!
Please update the docstring of this function using the newly created SortValidationError
, along with the two raise
statements at lines 143 and 147.
Co-authored-by: Alex Petenchea <[email protected]>
Co-authored-by: Alex Petenchea <[email protected]>
In order to merge, please make sure linter is passing
|
Done. Linter seems to be passing now. |
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.
LGTM
add support for sorting by multiple fields in collection.find function