Skip to content

Add missing Optional types in urllib.parse #3263

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

Merged
merged 1 commit into from
Oct 9, 2019

Conversation

robertschweizer
Copy link
Contributor

None values are accepted, and interpreted as empty (byte) strings by
some urllib.parse functions.

This is probably not the desired behavior, but can be useful especially
when passing through values that default to None in outside functions.

Signed-off-by: Schweizer, Robert [email protected]

None values are accepted, and interpreted as empty (byte) strings by
some urllib.parse functions.

This is probably not the desired behavior, but can be useful especially
when passing through values that default to None in outside functions.

Signed-off-by: Schweizer, Robert <[email protected]>
@gvanrossum
Copy link
Member

I see a problem though. When I call e.g. parse_qs(None) what's the return type?

@robertschweizer
Copy link
Contributor Author

It returns an empty dictionary, which fits the return type annotation and is the same behavior as for empty strings.

None is interpreted as an empty byte string though. E.g. urlparse(None) returns a ParseResultBytes instance.

@gvanrossum
Copy link
Member

Hm, that's unfortunate (choosing bytes is arbitrary). However this should occur rarely -- usually the static type of the argument will be Optional[bytes] or Optional[Text], not None.

I'll leave it to others to approve this PR.

@srittau
Copy link
Collaborator

srittau commented Oct 8, 2019

I am not sure whether we should change the types. Allowing None seems not to be a conscious design decision, but just a side-effect of using if x to check for an empty string in _decode_args(). Relying on this to work is one of the things that a type checker is supposed to prevent, in my opinion.

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Looking at this line in urllib/parse.py:

    return tuple(x.decode(encoding, errors) if x else '' for x in args)

that looks pretty intentional to skip None -- if x were b'', then x.decode(...) would return '' so there would be no need for the if x else '' part.

This is in _decode_args() which is called from _coerce_args() which is called from parse_qs() and from at least most of other functions touched in this PR (I didn't bother to check all).

So I think it's intentional. @robertschweizer is this how you found this? (By looking for callers of _coerce_args().) For anything that calls _coerce_args() I think this is by design (or at least it's not going to be changed out of fear of breaking existing usage).

@srittau srittau merged commit 3ee8fc2 into python:master Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants