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

feat: takes into account when statics not in host_url root #22

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,17 @@ jobs:

- name: "Install dependencies"
run: |
pip3 install flake8 Flask==1.1.2
pip3 install flake8 Flask==1.1.2 pytest
pip3 install -e .

- name: "Lint code base"
run: |
flake8 .

- name: "Run pytest suite"
run: |
python -m pytest tests/

- name: "Run test suite"
run: |
cd tests/example_app
Expand Down
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -81,3 +81,6 @@ coverage.xml
*.pot

# End of https://www.gitignore.io/api/python,osx

env/
.vscode/
37 changes: 32 additions & 5 deletions flask_static_digest/__init__.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
import json
import os

from urllib.parse import urljoin

from urllib.parse import urljoin, urlparse
from flask import url_for as flask_url_for


Expand All @@ -25,7 +24,8 @@ def init_app(self, app):
app.config.setdefault("FLASK_STATIC_DIGEST_GZIP_FILES", True)
app.config.setdefault("FLASK_STATIC_DIGEST_HOST_URL", None)

self.host_url = app.config.get("FLASK_STATIC_DIGEST_HOST_URL")
self.host_url, self.prefix = parse_digest_host_url(
app.config.get("FLASK_STATIC_DIGEST_HOST_URL"))
self.static_url_path = app.static_url_path

self.manifest_path = os.path.join(app.static_folder,
Expand All @@ -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
Copy link
Owner

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?

filter(None, [self.prefix, self.static_url_path, filename]))

return urljoin(self.host_url, "/".join(parts))

def static_url_for(self, endpoint, **values):
"""
Expand Down Expand Up @@ -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):
Copy link
Owner

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?

"""
Detect if host_url_prefix contains a path element and returns a tuple with
the elements (host_part, path_part), for example for
Copy link
Owner

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:
Copy link
Owner

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:

('https://cdn.example.com', "/myapp/some/path/")
Copy link
Owner

@nickjj nickjj Jul 10, 2021

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/")


:param host_url_prefix: CDN like URL prefix
:type host_url_prefix: str
:return: tuple with the elements (host_part, path_part).
"""
scheme, netloc, path, _, _, _ = urlparse(host_url_prefix)

if path and path.startswith("/"):
path = path[1:]

if path and path.endswith("/"):
path = path[:-1]

return (f"{scheme}://{netloc}", path)
39 changes: 39 additions & 0 deletions tests/test_parse_digest_host_url.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
from flask_static_digest import parse_digest_host_url
Copy link
Owner

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?


import pytest as pt


@pt.mark.parametrize(
"digest_host_url, expected", [
(
"https://cdn.example.com",
("https://cdn.example.com", "")
),
(
"https://cdn.example.com/",
("https://cdn.example.com", "")
),
(
"https://cdn.example.com/myapp",
("https://cdn.example.com", "myapp")
),
(
"https://cdn.example.com/myapp/",
("https://cdn.example.com", "myapp")
),
(
"https://cdn.example.com/myapp/anotherdir",
("https://cdn.example.com", "myapp/anotherdir")
),
(
"https://cdn.example.com/myapp/anotherdir/",
("https://cdn.example.com", "myapp/anotherdir")
),
]
)
def test_parse_function(digest_host_url, expected):
assert parse_digest_host_url(digest_host_url) == expected


def test_joined_urls():
pass
82 changes: 82 additions & 0 deletions tests/test_static_url_for.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
from flask import Flask
from flask_static_digest import FlaskStaticDigest
import pytest as pt
import json


fake_manifest = {
"hey.png": "hey-HASH.png",
"images/hey.png": "images/hey-HASH.png",
"images/card/hey.png": "images/card/hey-HASH.png"
}


def make_app(tmp_path, host_url, static_url_path):
appdir = tmp_path / 'fakeappdir'
appdir.mkdir()

staticdir = appdir / 'static'
staticdir.mkdir()

manifest_f = staticdir / 'cache_manifest.json'
manifest_f.write_text(json.dumps(fake_manifest))

app = Flask(
__name__, root_path=appdir, static_folder=staticdir,
static_url_path=static_url_path)
app.config['TESTING'] = True
app.config['FLASK_STATIC_DIGEST_HOST_URL'] = host_url

with app.app_context():
return app


@pt.mark.parametrize(
"host_url, static_url_path, file, expected", [
(
"https://cdn.example.com", None, "hey.png",
"https://cdn.example.com/static/hey-HASH.png"
),
(
"https://cdn.example.com", None, "images/hey.png",
"https://cdn.example.com/static/images/hey-HASH.png"
),
(
"https://cdn.example.com", "/static/something/", "hey.png",
"https://cdn.example.com/static/something/hey-HASH.png"
),
(
pt.param(
"https://cdn.example.com", "static/something/", "hey.png",
"https://cdn.example.com/static/something/hey-HASH.png",
marks=pt.mark.xfail(raises=ValueError)
)
),
(
"https://cdn.example.com/myapp", None, "images/hey.png",
"https://cdn.example.com/myapp/static/images/hey-HASH.png"
),
(
"https://cdn.example.com/myapp", '/static', "images/hey.png",
"https://cdn.example.com/myapp/static/images/hey-HASH.png"
),
(
"https://cdn.example.com/myapp", "/static/something/", "hey.png",
"https://cdn.example.com/myapp/static/something/hey-HASH.png"
),
(
"https://cdn.example.com/myapp/anotherdir", None, "images/hey.png",
"https://cdn.example.com/myapp/anotherdir"
"/static/images/hey-HASH.png"
),
(None, None, "hey.png", "/static/hey-HASH.png"),
(None, '/mystatic/url', "hey.png", "/mystatic/url/hey-HASH.png"),
nickjj marked this conversation as resolved.
Show resolved Hide resolved
]
)
def test_get_url(tmp_path, host_url, static_url_path, file, expected):
app = make_app(tmp_path, host_url, static_url_path)
ext = FlaskStaticDigest(app=app)

ret = ext.static_url_for('static', filename=file)

assert ret == expected