-
Notifications
You must be signed in to change notification settings - Fork 27
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
Allow for host urls to include paths #38
Conversation
flask_static_digest/__init__.py
Outdated
This function will join the host URL with another URL. | ||
It differs from urljoin in that it will keep the path of the host URL | ||
even if url is an absolute path. | ||
:param url: the URL returned by url_for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please change this to: :param url: The URL returned by url_for
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it to url_path as suggested in #38 (comment).
flask_static_digest/__init__.py
Outdated
) | ||
def _custom_url_join(self, url): | ||
""" | ||
This function will join the host URL with another URL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please change this entire description to:
Join the host URL with another URL path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
flask_static_digest/__init__.py
Outdated
return urljoin( | ||
self.host_url, "/".join([self.static_url_path, filename]) | ||
) | ||
def _custom_url_join(self, url): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please rename the url
argument to url_path
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This contradicts your suggestion in #38 (comment). Should I rename it to url_for
or url_path
?
flask_static_digest/__init__.py
Outdated
if self.host_url is None: | ||
return url | ||
|
||
return self.host_url.rstrip("/") + '/' + url.lstrip("/") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than concatenating the string with +
what do you think about "/".join([self.host_url.rstrip("/"), url.lstrip("/")])
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either work. Your suggestion is probably more "modern" for lack of a better term. I opted for the +
way of concatenating because it looked more intuitive and obvious what's returned to me. Another alternative would be using an f-string f'{self.host_url.rstrip("/")}/{url.lstrip("/")}'
. There's also the option to use the removesuffix("/")
and removeprefix("/")
instead of rstrip("/")
and lstrip("/")
and while I think those methods would be more fitting, they're relatively new and require Python 3.9 so I opted for the older version to achieve a better compatibility.
In any case, it's your repository so while I respect that you formulated it as a question, I'll ultimately refer to your choice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I'm dumb. Let's go with an f-string. Given Python 3.9 isn't end of life until October 2025 let's avoid the new functions for now.
Thanks for taking this on btw! Just so we have a reference, this will fix #21. |
You're very welcome. 🙂 It should, yes. The criticism I can see is that, like @ybenitezf mentions in #21 (comment), technically speaking, the This could possibly be fixed by introducing new variables for the different parts of the url (scheme, host, path etc.) and then combining those somehow, but all my attempts at making better use of base Python functionality to not only do the combining but also verify that the resulting url is valid, have yielded no results. In the end, I came to the conclusion that it's not worth complicating the process for the sake of semantics when the current solution works with some minor adjustments.
|
Yep, I that's why I added a With that said, given we always end up calling Flask's For now, I do agree leaving things as they are is a good first move. This at least fixes the issue of wanting to set a path as part of your URL. |
By the way, the failing pipeline isn't due to this PR. It's related to the CI environment using Python 3.8. Can you roll up all of your commits into a single commit message of: "Fix host url to support a URL path" and force push it? Once that's complete I'll merge this and fix the CI issue afterwards. |
Done, please let me know if you need anything else from me. |
That's it, thanks for the contribution! I just pushed v0.4.1 which has this fix. |
Ugh, I just realized I missed changing the name of the second |
I think it works as is, |
The urllib.parse.urljoin function will discard the path of the url if the joined path is absolute (i.e. starts with a slash).
E.g.
This poses a problem because flask.url_for will always return absolute paths and some cdn hosts like Google mandate a path to use their Cloud storage buckets as cdn. My solution suggests removing trailing and leading slashes from host and url respectively and then concatenating them with a slash.
Successfully tested with the following hosts: None, "", "https://cdn.example.com", "https://cdn.example.com/", "https://cdn.example.com/path", "https://cdn.example.com/path/", "https://cdn.example.com/path/to/static", "https://cdn.example.com/path/to/static/".