Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Make the URL.include_query_params() to support multiple query string parameters with the same name #1738

Closed
wants to merge 3 commits into from
Closed

Make the URL.include_query_params() to support multiple query string parameters with the same name #1738

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a3e4dba
URL.include_query_params() supports multiple query string parameters …
lexabug Jul 4, 2022
c3b0495
Rename the items parameter of URL.include_query_params() into __items
lexabug Jul 7, 2022
fa69228
Merge branch 'master' into fix-the-include-query-params
adriangb Nov 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion starlette/datastructures.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,8 +133,17 @@ def replace(self, **kwargs: typing.Any) -> "URL":
components = self.components._replace(**kwargs)
return self.__class__(components.geturl())

def include_query_params(self, **kwargs: typing.Any) -> "URL":
def include_query_params(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And how do you include a query param called "items"? 👀

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can think of about 3 solutions to this issue:

a. use positional-only parameters: def include_query_params(self, items, /, **kwargs): - however, that syntax only works in Python 3.8+, and we still want to support Python 3.7;
b. use an *args parameter (similar to ImmutableMultiDict __init__): def include_query_params(self, *args, **kwargs): - that looks like a fine solution, even if the signature becomes a bit harder to read;
c. define a new method, e.g. def append_query_params(self, items): - that solution also looks fine to me but makes the API a bit heavier;

An intermediary solution would be to define the API as in (b), but raise an error if more than one unnamed argument is passed, so that a migration to (a) becomes possible once support for Python 3.7 can be dropped;

Copy link
Member

@adriangb adriangb Jul 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a. use positional-only parameters: def include_query_params(self, items, /, **kwargs): - however, that syntax only works in Python 3.8+, and we still want to support Python 3.7;

We could do:

def include_query_params(self, __items, **kwargs):

This makes __items positional only as far as type checkers are concerned (see https://peps.python.org/pep-0484/#positional-only-arguments) and at runtime you just can't have a query parameter with the name __items, which I hope no one is using. Then when Python 3.7 because the minimum supported version we can enforce it at runtime with your version

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the target API is to use a positional parameter, I would rather switch to the def append_query_items(self, *args, **kwargs): syntax and check inside the method body that len(items) < 2 - that is, I prefer my breaking changes to be impossible rather than extremely unlikely. (E.g. there is also potential for someone who does not use a type checker to simply call the function with __items=[], which would break once __items was replaced with the / syntax)

However, that is a question of API design, and I do not know what policy (if any) Starlette has for such API changes.

self,
__items: typing.Optional["ImmutableMultiDict[str, str]"] = None,
**kwargs: typing.Any,
) -> "URL":
params = MultiDict(parse_qsl(self.query, keep_blank_values=True))

if __items:
for key, value in __items.multi_items():
params.append(str(key), str(value))

params.update({str(key): str(value) for key, value in kwargs.items()})
query = urlencode(params.multi_items())
return self.replace(query=query)
Expand Down
16 changes: 16 additions & 0 deletions tests/test_datastructures.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,22 @@ def test_url_query_params():
assert str(u) == "https://example.org/path/?page=4"
u = u.include_query_params(search="testing")
assert str(u) == "https://example.org/path/?page=4&search=testing"
same_parameters = MultiDict()
same_parameters.append("list", 1)
same_parameters.append("list", 2)
same_parameters.append("list", 3)
u = u.include_query_params(same_parameters)
assert (
str(u) == "https://example.org/path/?page=4&search=testing&list=1&list=2&list=3"
)
same_parameters = MultiDict()
same_parameters.append("list2", 1)
same_parameters.append("list2", 2)
u = u.include_query_params(same_parameters, list2="abc")
assert (
str(u) == "https://example.org/path/?page=4&search=testing&"
"list=1&list=2&list=3&list2=abc"
)
u = u.replace_query_params(order="name")
assert str(u) == "https://example.org/path/?order=name"
u = u.remove_query_params("order")
Expand Down