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

Fix: Use urllib to combine urls (#5) #22

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

@erenes
Copy link

@erenes erenes commented Apr 5, 2021

No longer does the lack of slash at the end of api- or tus-url cause an issue while using the cli.

@erenes
Copy link
Author

@erenes erenes commented Apr 6, 2021

At least with this one I was able to trick all the checks into passing ;-)

@LordAro
Copy link
Member

@LordAro LordAro commented Apr 6, 2021

Not a huge issue, but the email address used in the commit is not attached to your GH account - you may wish to fix this (either by redoing the commits with a different email address or adding that email address to GH

@Xaroth
Copy link

@Xaroth Xaroth commented Apr 6, 2021

One thing to keep in mind is that urljoin has a different behavior than what was used.

Current:

>>> path = "/bla/something/"
>>> f"https://google.com/test{path}"
'https://google.com/test/bla/something/'

New:

>>> urljoin("https://google.com/test", "/bla/something/")
'https://google.com/bla/something/'
>>>

This effectively breaks use-cases where tus_url/api_url ends on a directory rather than just a domain.

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Apr 6, 2021

What I did for other places, what might be a way to avoid the weird urllib.parse functions, is that I check if an URL ends with a / when loading the setting, and add it if it is not there. But I haven't really looked into this yet, so this is just a random remark that might completely miss the goal :D

@erenes
Copy link
Author

@erenes erenes commented Apr 6, 2021

What I did for other places, what might be a way to avoid the weird urllib.parse functions, is that I check if an URL ends with a / when loading the setting, and add it if it is not there. But I haven't really looked into this yet, so this is just a random remark that might completely miss the goal :D

That's plan B, I'll look into urllib.parse pecularities and if I can't figure out a way to support paths with a directory I'll go for that approach. Also I'll squash the commits and fix my e-mail address :)

@erenes
Copy link
Author

@erenes erenes commented Apr 6, 2021

Based on these tests, I have updated the code to always add a trailing slash to the url, and remove the slash-prefix in all calls to session.x methods:

>>> import urllib.parse
>>> urllib.parse.urljoin('http://openttd.org/xx/yy/zz/', 'blaaaaaatt')
'http://openttd.org/xx/yy/zz/blaaaaaatt'
>>> urllib.parse.urljoin('http://openttd.org/xx/yy/zz', 'blaaaaaatt')
'http://openttd.org/xx/yy/blaaaaaatt'
>>> urllib.parse.urljoin('http://openttd.org/xx/yy/zz', '/blaaaaaatt')
'http://openttd.org/blaaaaaatt'
>>> urllib.parse.urljoin('http://openttd.org/xx/yy/zz///', '/blaaaaaatt')
'http://openttd.org/blaaaaaatt'
>>> urllib.parse.urljoin('http://openttd.org/xx/yy/zz///', 'blaaaaaatt')
'http://openttd.org/xx/yy/zz/blaaaaaatt'

commit coming soon :)

Copy link
Member

@TrueBrain TrueBrain left a comment

You now basically did both solutions, where either one works too :D That is a bit too much in my opinion.

In general, everyone tries to avoid urllib as it is just horrible in so many ways. One of the oldest Python libraries, and it really shows. If possible, I would be really happy if we can avoid using it.

I think that the change you made with the slashes is sufficient. That means urllib is basically not doing anything anymore, as the links are (after my suggestion) always correct already. Also saves some CPU cycles \o/ (no, that is a joke, that is never a goal here :D)

(and in case you wonder, I consider it horrible because (personal opinion incoming): include-path is weird, you have to include urllib.path, it does magic things with your strings without being really verbose why (which is shown by the fact you had to experiment with it to know what it did :P), and most of all: in 90% of the cases it can be avoided :D )

self.api_url = api_url
self.tus_url = tus_url
self.api_url = f"{api_url}/"
self.tus_url = f"{tus_url}/"
Copy link
Member

@TrueBrain TrueBrain Apr 12, 2021

Choose a reason for hiding this comment

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

I would do:

if not api.url.endswith("/"):
  api_url += "/"

or something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants