-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
[utils] base_url: URL paths can contain &. Backport from yt-dlp Resolves #31485 #31490
base: master
Are you sure you want to change the base?
Conversation
Please fill in the description and include the magic line Anyway, why is |
Also, the change does have a test, so you can tick that checkbox too. |
I don't know. I didn't write any of this code. I'm just trying to get it to work. "Anyway, why is base_url() using regex to parse URLs when there's an actual Python library module for that?" |
Likewise. It was really a rhetorical question: the regex must have been an easy win. Something like this makes it clearer what's being removed and what kept. It's not obvious what the right answer for (eg) def base_url(url):
"""
Return the URL, with its path trimmed after the rightmost /
and without any query, params or fragment
"""
parsed_url = compat_urlparse.urlparse(url)
path = parsed_url.path.rsplit('/')[0]
# don't restore trailing //: correct?
if not path.endswith('/'):
path += '/'
return compat_urlparse.urlunparse(parsed_url._replace(
path=path, query='', params='', fragment='') |
Of course the "right answer" is what def test_base_url(self):
self.assertEqual(base_url('http://foo.de/'), 'http://foo.de/')
self.assertEqual(base_url('http://foo.de/bar'), 'http://foo.de/')
self.assertEqual(base_url('http://foo.de/bar/'), 'http://foo.de/bar/')
self.assertEqual(base_url('http://foo.de/bar/baz'), 'http://foo.de/bar/')
self.assertEqual(base_url('http://foo.de/bar/baz?x=z/x/c'), 'http://foo.de/bar/') |
And also, RFC 3986 2.2 categorises
The principle is that a
In other words, |
Please approve this PR, the error is affecting many systems @dirkf |
Please follow the guide below
x
into all the boxes [ ] relevant to your pull request (like that [x])Before submitting a pull request make sure you have:
In order to be accepted and merged into youtube-dl each piece of code must be in public domain or released under Unlicense. Check one of the following options:
What is the purpose of your pull request?
Description of your pull request and other information
Explanation of your pull request in arbitrary form goes here. Please make sure the description explains the purpose and effect of your pull request and is worded well enough to be understood. Provide as much context and examples as possible.
Resolves #31485