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

/-joining of urls can produce multiple slashes #12

Closed
mgedmin opened this issue Sep 17, 2016 · 6 comments
Closed

/-joining of urls can produce multiple slashes #12

mgedmin opened this issue Sep 17, 2016 · 6 comments

Comments

@mgedmin
Copy link
Contributor

mgedmin commented Sep 17, 2016

>>> from yarl import URL
>>> URL('http://example.com/api/') / 'sub/path'
URL('http://example.com/api//sub/path')

Should that double slash be there?

Other corner cases:

>>> URL('http://example.com/api') / '/sub/path'
URL('http://example.com/api//sub/path')
>>> URL('http://example.com/api/') / '/sub/path'
URL('http://example.com/api///sub/path')

These examples work the way I expect:

>>> URL('http://example.com/') / 'sub/path'
URL('http://example.com/sub/path')
>>> URL('http://example.com') / 'sub/path'
URL('http://example.com/sub/path')

These do not:

>>> URL('http://example.com/') / '/sub/path'
URL('http://example.com//sub/path')
>>> URL('http://example.com') / '/sub/path'
URL('http://example.com//sub/path')

Bug or explicit design decision?

@mgedmin mgedmin changed the title /-oining of urls can produce multiple slashes /-joining of urls can produce multiple slashes Sep 17, 2016
@asvetlov
Copy link
Member

Thank for pointing on quirks out!
URL('http://example.com/api/') / 'sub/path' shouldn't produce double slash.
But appending a path starting from slash should be forbidden explicitly I think.
For URL('http://example.com/api') / '/sub/path' case the right part looks like an absolute path, appending absolute path to absolute URL is wrong from my perspective.

Would you make a PR with fix?

@mgedmin
Copy link
Contributor Author

mgedmin commented Sep 17, 2016

I'm ambivalent about URL(...) / '/subpath/starting/with/a/slash'. Sometimes it seems to make sense, e.g. when you're considering a WSGI application mounted at some URL prefix, and you're thining about API endpoints in terms of /path/from/the/app/root. I don't personally care strongly about it. (I don't in fact use yarl anywhere yet. 😉)

@asvetlov
Copy link
Member

Let's forbid it now.
If we'll change our mind in future we may relax the rule without any backward incompatibility issue.
But restricting is much more painful process :)

@mgedmin
Copy link
Contributor Author

mgedmin commented Sep 17, 2016

Do you want me to update my PR, or ... ?

(I'm feeling kind of lazy.)

@asvetlov
Copy link
Member

If you would to update it or create new one with preventing absolute paths -- please go ahead.
If not -- I'll just merge existing code and will try to find a time for fixing by myself.

@mgedmin
Copy link
Contributor Author

mgedmin commented Sep 17, 2016

Go ahead and merge then.

Thanks!

asvetlov pushed a commit that referenced this issue Sep 17, 2016
* Add a tox.ini

Tox is an incredibly convenient tool that manages multiple virtualenvs
for you, so you can test against multiple Python versions with one
command.

* Avoid doubling slashes when joining paths

See #12.
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

No branches or pull requests

2 participants