Skip to content

Fix issue #255, create function url_join #304

Closed
wants to merge 6 commits into from

5 participants

@k21
k21 commented Jan 6, 2012

No description provided.

@kemitche kemitche and 1 other commented on an outdated diff Jan 6, 2012
r2/r2/lib/utils/utils.py
@@ -1372,3 +1372,31 @@ def constant_time_compare(actual, expected):
result |= ord(actual[i]) ^ ord(expected[i % expected_len])
return result == 0
+def url_join(*parts):
+ """
+ Join one or more URL components. Slashes are added between each two
+ components. Extra slashes are removed.
+ """
+ path = []
+ for p in parts:
+ if p == '': continue
+
+ if p[0] == '/':
@kemitche
kemitche added a note Jan 6, 2012

1384 - 1398 can be simplified to:
p.strip("/")

@k21
k21 added a note Jan 6, 2012

Thanks, I will change it. I feel stupid :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@kemitche
kemitche commented Jan 6, 2012

Looks great!

@spladug
reddit member
spladug commented Jan 6, 2012

Very nice. There are a ton of other places that use os.path.join incorrectly, would you be interested in throwing a few more in here? I'm happy to merge as-is if not.

@k21
k21 commented Jan 6, 2012

I personally expected to find more of them. When I grepped the sources for 'os.path.join' or 'path.join' I have found only 29 occurrences so I checked them all manually and it looked like all the others are used for files. Are you sure there are a ton of places? I still cannot find them.

@spladug
reddit member
spladug commented Jan 6, 2012

Wow, you're right. It's not as bad as I thought. It does seem that template_helpers.py:93 and js.py:78 could be url_join'd though.

@k21
k21 commented Jan 7, 2012

I have noticed a problem, if the url starts with a slash, this slash is removed, which changes absolute path to relative. I am going to fix it and change the two locations.

@k21
k21 commented Jan 7, 2012

I have noticed that the function in template_helpers.py does sometimes call os.path.join with parameters like ['/static/', '/static/js', 'utils.js']. It relies on the fact that if there is a absolute path, all paths before it are ignored. Should this be fixed in url_join or in the caller?

@k21
k21 commented Jan 7, 2012

For now, I changed url_join to behave more like os.path.join.

@bboe
bboe commented Jan 7, 2012

Just curious, what's wrong with urlparse.urljoin?

No solution is going to handle everything, for instance if you forget the trailing slash for a directory then it will be stripped off in the case of:

>>> urlparse.urljoin('foo', 'bar')
'bar'

Likewise having leading slashes on the latter part will truncate the remainder, however, I think that's to be expected.

>>> urlparse.urljoin('foo/', '/bar')
'/bar'

I see that one condition is you want to join more than two parameters. So maybe use urlparse.url join inside the iterator?

@k21
k21 commented Jan 7, 2012

@bboe: I did not check it thoroughly, but it only supports 2 parameters, while some invocations need to pass more, and the first parameter has to be a complete valid url, so it probably should contain things like schema, hostname etc., which is also not what we want.

What I want to say that there are two solutions to this problem: change uses of os.path.join to something with different behavior and make sure that the usage is correct in all the sites OR create a new function that behaves similarly to the old function, but it is platform-independent and semantically correct, and then we only have one function to check.

@k21
k21 commented Jan 9, 2012

I created another alternative which uses string concatenation instead of join, so it is more readable. It does possibly have worse performance, but it should not matter for the few strings for which it is used now. Please look here and tell me if it is acceptable, thanks :)

@chromakode

Nice! The new function and its invocations look good to me. I think it's good to include the absolute URL handing behavior, since it is useful in URL building functions like static. I like the new version, though it looks like it'd be simple enough to turn it into an array based string builder again.

An aside: while taking a look around template_helpers.py, I noticed a function called join_urls. It looks like it might be trivial to remove this duplicate function and replace it with your new util.

@spladug
reddit member
spladug commented Jul 16, 2013

Unfortunately, due to our latency in merging this, it's become unmergeable and out of date. I'm going to close this. Thanks a tonne for your work and we should definitely still do something like this.

@spladug spladug closed this Jul 16, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.