-
Notifications
You must be signed in to change notification settings - Fork 2
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
OPT: Support list query parameters #133
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #133 +/- ##
==========================================
+ Coverage 92.39% 92.43% +0.03%
==========================================
Files 33 33
Lines 855 859 +4
Branches 129 131 +2
==========================================
+ Hits 790 794 +4
Misses 45 45
Partials 20 20 ☔ View full report in Codecov by Sentry. |
@@ -117,6 +117,11 @@ class QueryParam(ParameterMarker[ContainsQueryParams]): | |||
|
|||
@override | |||
def __call__(self, request: ContainsQueryParams, value: Any) -> None: # noqa: D102 | |||
if isinstance(value, list): |
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'd actually prefer to accept any iterable here (say, a tuple). The docs say the most reliable way would be:
try:
values = iter(value)
except TypeError:
... # old code for scalar values
else:
for sub_value in values:
...
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.
Yeah i thought about accepting any iterable, however that would also cover strings which we don't want to iter through here.
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.
Oh, good one, didn't think of strings. They'd also pass isinstance(..., Collection)
check
Also, please don't care fixing the 3.8 checks – it's end-of-life, I'll remove the support completely |
Before this change it would URL encode the python representation of the list.
With this change it will pass the query parameter as
?multivalue=value1&multivalue=value2
I was doubting whether to use
isinstance(.., list)
vs something more generic likeisinstance(.., Collection)
, but i believe list is the most appropriate typing. It has order, and it clearly defines what would be expected when passing a list as a query parameter.