Improve filtering #121
Merged
Improve filtering #121
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@rbagrov could you remove the Also, if you could rebase from N.B. I removed the link to clickup as this is a public repo, but now I realize that no-one outside of the G-org would be able to visit it; (I put it back 😅). |
rbagrov
force-pushed
the
fix_query_filter_removal
branch
from
c24b2de
to
aebfc3f
Compare
|
Hey @marz619 :)
Please let me know when you have a chance to review/test the proposed changes, if there is anything I can improve. |
* remove the use of the class Empty as it is incompatible with py2 The idea behind this to preserve the state of the _current_ Query object in regards to modifying the _filters. !Breaking changes! .all() will *not* clear the filters .count() will *not* clear the filters
* Add docs where possible * consistent formatting across the file * move limit arg check in Query.all * Add a TypeError return * simplifies the method call to islice * adds test for bad type args
marz619
force-pushed
the
fix_query_filter_removal
branch
from
bb81d90
to
377b83f
Compare
rafikdraoui
approved these changes
Apr 13, 2020
marz619
added a commit
that referenced
this pull request
Apr 14, 2020
-------------------- ! BREAKING CHANGES ! -------------------- * The ``Query.filter`` method will return a clone/copy of itself. This will preserve the state of ``filters`` on the original Query object. * The ``Query.all`` method will **not** clear the filters after returning. * The ``Query.all`` method will return a ``TypeError`` if a type other than an ``int`` is passed to the ``limit`` argument. * The ``Query.count`` method will **not** clear the filters after returning. * see: #121 * The ``AgencyChain.agencies`` field will be returned as a list of Resource objects * see: f34afd52
marz619
added a commit
that referenced
this pull request
Apr 14, 2020
This is a result of a change introduced in 2.20.0 in Query.all
>> We check if the raw_data has an href and use that. This was because
of the the bookings/{id}/requirements endpoint actually returns
RequirementSet resources and the default behaviour would build the URI
as /bookings/{id}/requirement_sets
We will preserve the change from 2.20.0 and make the following
modification to the `APIRequestor.list_raw` method. Check the `uri`
passed in for a '?' and make the assumption that if the '?' is in the
URI, then we will not use the `params` the requestor may have been
initialised with.
+ Adds test cases to ensure params are not passed when a ? is present in
the URI
marz619
added a commit
that referenced
this pull request
Apr 17, 2020
This is a result of a change introduced in 2.20.0 in Query.all
>> We check if the raw_data has an href and use that. This was because
of the the bookings/{id}/requirements endpoint actually returns
RequirementSet resources and the default behaviour would build the URI
as /bookings/{id}/requirement_sets
We will preserve the change from 2.20.0 and make the following
modification to the `APIRequestor.list_raw` method. Check the `uri`
passed in for a '?' and make the assumption that if the '?' is in the
URI, then we will not use the `params` the requestor may have been
initialised with.
+ Adds test cases to ensure params are not passed when a ? is present in
the URI
marz619
added a commit
that referenced
this pull request
Apr 17, 2020
…123) * FIX-#121 - modify APIRequestor.list_raw to check URI for params * This is a result of a change introduced in 2.20.0 in Query.all >>> We check if the raw_data has an href and use that. This was because of the the bookings/{id}/requirements endpoint actually returns RequirementSet resources and the default behaviour would build the URI as /bookings/{id}/requirement_sets We will preserve the change from 2.20.0 and make the following modification to the `APIRequestor.list_raw` method. Check the `uri` passed in for a '?' and make the assumption that if the '?' is in the URI, then we will not use the `params` the requestor may have been initialised with. + Adds test cases to ensure params are not passed when a ? is present in the URI * use `urlparse` to determine if the uri contains query parameters * use `namedtuple` to store `parent` attribute This will still allows any code of the form: # destructure namedtuple u, i, vi = x.parent # index access u, i, vi = x.parent[0], x.parent[1], x.parent[2] to continue to work correctly
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Task: Click
Description: Query methods ( .filter() and .count() ) were clearing _filters attribute which resulted in single usage of a filtered query instance.
Reproduce:
and
Solution: Mimic Django ORM approach as close as possible