bpo-27485: Rename and deprecate undocumented functions in urllib.parse #2205
Conversation
Lib/lib2to3/fixes/fix_urllib.py
Outdated
| "splittype", "splituser", "splitvalue", ]), | ||
| "urlencode", "_splitattr", "_splithost", "_splitnport", | ||
| "_splitpasswd", "_splitport", "_splitquery", "_splittag", | ||
| "_splittype", "_splituser", "_splitvalue", ]), |
There was a problem hiding this comment.
I didn’t see a test case, but won’t this change make 2to3 code call the internal unsupported functions? I think it is better to keep calling the original functions, even if they are deprecated.
There was a problem hiding this comment.
Yes, thank you, this makes more sense.
Lib/urllib/parse.py
Outdated
| @@ -908,7 +909,14 @@ def urlencode(query, doseq=False, safe='', encoding=None, errors=None, | |||
| l.append(k + '=' + elt) | |||
| return '&'.join(l) | |||
|
|
|||
| def to_bytes(url): | |||
|
|
|||
| def to_bytes(url=''): | |||
There was a problem hiding this comment.
Does the signature have to change?
There was a problem hiding this comment.
When I was researching the codebase to see what changes needed to be made for a deprecation warning, I found dist and linux_distribution in platform and test_platform, so I copied the tests from there. That change altered the signature from the original to the internal version of the function. I didn't understand why, so I applied a similar change and I probably shouldn't have. I've removed it.
Lib/urllib/parse.py
Outdated
| @@ -982,7 +1037,15 @@ def splitport(host): | |||
| return host, port | |||
| return host, None | |||
|
|
|||
| def splitnport(host, defport=-1): | |||
|
|
|||
| def splitnport(url=''): | |||
There was a problem hiding this comment.
This was just careless. Sorry that I made the mistake. I've corrected all the signatures.
Lib/test/test_urlparse.py
Outdated
| @@ -1053,7 +1054,7 @@ def test_splitport(self): | |||
| self.assertEqual(splitport(':88'), ('', '88')) | |||
|
|
|||
| def test_splitnport(self): | |||
| splitnport = urllib.parse.splitnport | |||
| splitnport = urllib.parse._splitnport | |||
There was a problem hiding this comment.
If you tested the proper function, it would reveal that the new signature is incompatible.
There was a problem hiding this comment.
I understand this better now. Thank you for reviewing and for your comments.
Lib/urllib/parse.py
Outdated
| return _splitnport(url) | ||
|
|
||
|
|
||
| def _splitnport(host, defport=-1): |
There was a problem hiding this comment.
This does not seem to be used anywhere else. Why not merge the two functions?
There was a problem hiding this comment.
Do you mean have splitport and splitnport call one internal function?
Lib/urllib/parse.py
Outdated
|
|
||
| def splitvalue(url=''): | ||
| warnings.warn("urllib.parse.splitvalue() is deprecated as of 3.7, " | ||
| "use urllib.parse.urlparse() instead", |
There was a problem hiding this comment.
I don’t see how urlparse is a substitute for this
There was a problem hiding this comment.
Thank you. You're right, urlparse wasn't a good substitute. I was thinking that the query part of the result from urlparse would be similar to splitvalue, but that's closer to splitquery and still would need extra parsing. I've changed it to use urllib.parse.parse_qsl.
|
Looks like this unfortunately didn't make it into 3.7. There's a conflict that would need to be solved and the deprecation messages would need to be renamed to mention "3.8". |
|
@ambv Thank you for the review. I've rebased and updated the deprecation messages to reference 3.8 instead of 3.7. |
|
Thanks! ✨ 🍰 ✨ |
| urllib.parse.splittype('') | ||
| self.assertEqual(str(cm.warning), | ||
| 'urllib.parse.splittype() is deprecated as of 3.8, ' | ||
| 'use urllib.parse.urlparse() instead') |
There was a problem hiding this comment.
From https://docs.python.org/3.8/library/urllib.parse.html#urllib.parse.urlsplit:
This is similar to
urlparse(), but does not split the params from the URL. This should generally be used instead ofurlparse()if the more recent URL syntax allowing parameters to be applied to each segment of the path portion of the URL (see RFC 2396) is wanted.
Based on this, looks like it's better to recommend using urllib.parse.urlsplit() instead of urllib.parse.urlparse().
What do you think?
The following functions were undocumented:
splitattr,splithost,splitnport,splitpasswd,splitport,splitquery,splittag,splittype,splituser,splitvalue,to_bytes,unwrapA note in the 2.7 documentation for
urllibstated that Python 3 does not expose these helper functions, but yet they were still available.https://bugs.python.org/issue27485