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.

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

Ability to remove a query value while retaining others #774

Closed
1 task done
apmorton opened this issue Oct 5, 2022 · 4 comments · Fixed by #898
Closed
1 task done

Ability to remove a query value while retaining others #774

apmorton opened this issue Oct 5, 2022 · 4 comments · Fixed by #898

Comments

@apmorton
Copy link

apmorton commented Oct 5, 2022

Is your feature request related to a problem?

I have a situation where I would like to get a query parameter, do something with it, and then pass the URL onto a lower layer without that single query param.

The current API is awkward for this.

Describe the solution you'd like

It would be nice to be able to pass kwargs to update_query with None value and have that remove those specific query params.

actual = URL('http://test.com/path?a=1&b=2').update_query(a=None)
expected = URL('http://test.com/path?b=2')
assert actual == expected

Today this throws a TypeError

Describe alternatives you've considered

Today you can accomplish this, but its somewhat awkward:

url = URL('http://test.com/path?a=1&b=2')
query = url.query.copy()
del query['a']
actual = url.with_query(query)
expected = URL('http://test.com/path?b=2')
assert actual == expected

Additional context

No response

Code of Conduct

  • I agree to follow the aio-libs Code of Conduct
@webknjaz
Copy link
Member

webknjaz commented Dec 3, 2022

None seems ambiguous. Its semantics can be misinterpreted. JS would use null in it's place and that would be transmitted over the wire as a string, for example.
The None- check is correct.
I think you could just use an empty string and semantically it'd be the same as an unset value, usually.
But if you want to really exclude it, I'd recommend going for a separate method for deleting the param rather than setting it to a magical sentinel value which would mean "instead of updating, remove".
If you want to take a stab at it — send a PR with tests for review.

@apmorton
Copy link
Author

apmorton commented Dec 3, 2022

Assigning an argument to none None seems logical and consistent to me given the existing behavior of passing None to with_query and update_query.

A separate method is fine too, but given None explicitly has no meaning now (by way of TypeError) it seemed a good fit.

A single method is definitely more convenient - imagine the use case of mutating and renaming an existing query parameter.

url.update_query(old=None, new=func(url.query['old']))

vs

url.update_query(new=func(url.query['old'])).remove_query('old')

Either is an improvement, but I think the first is more ergonomic.

@webknjaz
Copy link
Member

Assigning an argument to none None seems logical and consistent to me given the existing behavior of passing None to with_query and update_query.

I disagree: for with_query(), the value of None can have the semantics of "no query" as it works with complex mappings. When working with a value of a param, the value is a primitive. None is ambiguous in this case.

As for update_query(), I think allowing None was a design mistake in that context since it's advertised as a way to patch individual params and this magic sentinel just removes them all.

@webknjaz webknjaz linked a pull request Nov 20, 2023 that will close this issue
4 tasks
@webknjaz
Copy link
Member

I also stumbled upon an earlier discussion mentioning the ambiguity of None in this very same context: #307 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants