-
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
feat: takes into account when statics not in host_url root #22
Conversation
Hi, Thanks, I haven't gotten a chance to test it locally but does this work when |
I just wanted to test this separately. Also, It seemed to me that I should not mix |
Will have to think about that one a bit. Just replying to acknowledge your response. Since it's the work week now I'm a little slammed, but I want to let you know this will for sure get merged in 1 way or another. |
@nickjj ¿any progress with this? |
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.
Sorry, I've left a few comments. Let me know what you think.
def parse_digest_host_url(host_url_prefix): | ||
""" | ||
Detect if host_url_prefix contains a path element and returns a tuple with | ||
the elements (host_part, path_part), for example 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 re-word this line to be:
the elements (host_part, path_part), for example:
|
||
host_url_prefix = "https://cdn.example.com/myapp/some/path/" | ||
|
||
This function should return: |
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 change this line to:
This function will return results such as:
host_url_prefix = "https://cdn.example.com/myapp/some/path/" | ||
|
||
This function should return: | ||
('https://cdn.example.com', "/myapp/some/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.
Can you change this line into these lines:
# When there's no path:
("https://cdn.example.com")
# When there's a path:
("https://cdn.example.com", "/my/custom/path/")
@@ -84,3 +86,28 @@ def static_url_for(self, endpoint, **values): | |||
merged_values.get("filename")) | |||
else: | |||
return flask_url_for(endpoint, **merged_values) | |||
|
|||
|
|||
def parse_digest_host_url(host_url_prefix): |
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 rename this function to _parse_digest_host_url
since it's private?
@@ -46,8 +46,10 @@ def _load_manifest(self, app): | |||
return manifest_dict | |||
|
|||
def _prepend_host_url(self, host, filename): | |||
return urljoin(self.host_url, | |||
"/".join([self.static_url_path, filename])) | |||
parts = list( # removing empty strings |
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.
Any chance we can write rewrite this and the line below it to be a more clear on what it does?
@@ -0,0 +1,39 @@ | |||
from flask_static_digest import parse_digest_host_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.
Thoughts on replicating all of these tests without using pytest, following the same patterns used in the existing test suite?
I'm beginning to think this PR along with That completely negates needing What do you think? |
It depends on if it works, have you tested ? i don't have time right now, if u think that work ok, u should update the README.md and close this PR. |
I haven't tested it yet but I will in the next couple of days. If it works I'll update the readme and ship a new version with that option removed along with adding new docs to go over how to do the same thing with Flask's built in |
Nope, it doesn't work. Which means the option and this PR is still going to be a good idea. |
This has been resolved in: #38 |
I added some tests with
pytest
, think I included all the cases, let me know if i need to include more examplesCloses #21