GeezerVison

It's maybe a bit surprising to have some of the tuple fields sometimes be None (typing.Tuple[typing.Optional[str]] instead of typing.Tuple[str]), but I'm not sure of a more obvious solution.

The other alternative I thought about was to just explicitly dump in the delimiter if it's empty (e.g. 'http://example.com/?#' → 'http', 'example.com', '/', '?', '#'), but that's probably more surprising, rebuilding the URL is more complex, and what then if there's a URL like http://example.com/??.

I think you need to also describe the breaking change very clearly (haven't done it before, but I think that's what bedevere/news is for, i.e. these things), and leave hints in the actual documentation about the change ("changed in 3.9")

Housekeeping: I'd squash all the commits.

Thank you for reviewing this @nicktimko! Yes the None solution for undefined query/fragment seemed the most straightforward and natural to me.

I have updated the PR description to detail the exact changes. Nice suggestion, I will make the news entry, documentation version note and commit squash. But before I would like to fix an issue: the documentation tests in Travis CI failed for an obscure reason (see below). Do you have any idea why?

I don't know, but the docs build looks like it's installing blurb, which might be related to the news, so maybe adding a news item would fix it? Does it run locally? Just guessing though.

Thanks @nicktimko, I have added a news entry, but documentation tests still fail in Travis-CI.

I don't think the documentation failure is related to the code in this PR. Perhaps this PR needs to be rebased?

Thanks for reviewing this @orsenthil.

@nicktimko @orsenthil I have identified why the documentation tests and code coverage fail in Travis CI when running pip install.

This is because the _remove_md5_fragment function of the __init__ module of the pkg_resources package vendored with pip calls the startswith method on the fragment component of its parsed location parameter. But since with this PR it is no more '' but None, this fails and subsequently raises the AttributeError: 'PathMetadata' object has no attribute 'hashcmp' error.

The pip/_vendor/pkg_resources/__init__.py file should be updated like this to be compliant with this PR:

  def _remove_md5_fragment(location):
      if not location:
          return ''
      parsed = urllib.parse.urlparse(location)
-     if parsed[-1].startswith('md5='):
-         return urllib.parse.urlunparse(parsed[:-1] + ('',))
+     if parsed[-1] is not None and parsed[-1].startswith('md5='):
+         return urllib.parse.urlunparse(parsed[:-1] + (None,))

So as you said @orsenthil, this PR is a breaking change.

Adding a note again, this breaking change can be very costly

the urlsplit function now decodes an absent '' query component as None and an absent '' fragment component as None

Since a lot of packages depend upon urlparse for parsing and they suddenly geting a None value instead of expected '' empty string will break significant number of packages and applications.

I am still evaluating of this is bringing any benefit or is there a backwards compatible way. If there is none, I will be -1 on this patch.

@orsenthil Yes since this PR requires pip to be patched (cf. my previous comment) and potentially other third-party packages relying on the current urllib.parse behaviour, this PR is very dangerous and should not be merged, unless we have an update strategy for pip or we find a backward compatible way.

What is the result of your analysis? Do you think we should move this PR forward or we should close it because its benefits (RFC 3986 compliance) do not outweigh its costs?

Based on Senthil's last comment, I suggest closing this PR.

Can always engage in some fence-sitting and support both compatible and compliance options. Pass/set a flag, use a different function/method. How about a new urllib2 😉?

Re: returning None and how it would break things, for compatiblity, some ✨Magic™️ objects (or even just a uniquely referenced sentinel string?) could be used and otherwise work like an empty string but is some sentinel object for checking.

@nicktimko Yep, discussing possible solutions is always welcome, but please do so on the issue. Discussions on the PR should be focused on implementation specific issues. (Note that I only suggested to close this PR, not the linked issue.)

Like @erlend-aasland suggested, I am closing this PR because it is backward incompatible. I am also closing the associated issue #82150 which was too restricted (the problem also affects the authority component of URIs, as well as the urlparse() and urldefrag() functions in the urllib.parse module), poorly explained, and could not be edited since I had originally opened it on Round Up, before the migration to GitHub Issue.

I have just opened a new issue describing the problem more generally and clearly with a candidate solution based on @nicktimko’s last comment (feedback is welcome): #99962

Thank you @orsenthil and @nicktimko for reviewing this PR.