-
Notifications
You must be signed in to change notification settings - Fork 29
Change bundle path and fix relative URL's #41
base: master
Are you sure you want to change the base?
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -70,7 +70,9 @@ def get_js_urls(bundle, debug=None): | |
| bundle_full = 'js:%s' % bundle | ||
| if bundle_full in BUNDLE_HASHES: | ||
| build_id = BUNDLE_HASHES[bundle_full] | ||
| return (_get_item_path('js/%s-min.js?build=%s' % (bundle, build_id,)),) | ||
| item = 'js/%s-min.js?build=%s' % (bundle, build_id,) | ||
| item = os.path.join(getattr(settings, 'BUNDLES_DIR', ''), item) | ||
| return (_get_item_path(item),) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why again do we return a tuple of one item? |
||
|
|
||
|
|
||
| def _get_compiled_css_url(item): | ||
|
|
@@ -120,8 +122,9 @@ def get_css_urls(bundle, debug=None): | |
| bundle_full = 'css:%s' % bundle | ||
| if bundle_full in BUNDLE_HASHES: | ||
| build_id = BUNDLE_HASHES[bundle_full] | ||
| return (_get_item_path('css/%s-min.css?build=%s' % | ||
| (bundle, build_id)),) | ||
| item = 'css/%s-min.css?build=%s' % (bundle, build_id) | ||
| item = os.path.join(getattr(settings, 'BUNDLES_DIR', ''), item) | ||
| return (_get_item_path(item),) | ||
|
|
||
|
|
||
| @register.function | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,10 +32,12 @@ class Command(BaseCommand): # pragma: no cover | |
| checked_hash = {} | ||
| bundle_hashes = {} | ||
|
|
||
| tmp_files = [] | ||
| missing_files = 0 | ||
| minify_skipped = 0 | ||
| cmd_errors = False | ||
| ext_media_path = os.path.join(get_media_root(), 'external') | ||
| bundles_dir = getattr(settings, 'BUNDLES_DIR', '.') | ||
|
|
||
| def update_hashes(self, update=False): | ||
| def media_git_id(media_path): | ||
|
|
@@ -76,12 +78,17 @@ def handle(self, **options): | |
| for ftype, bundle in settings.MINIFY_BUNDLES.iteritems(): | ||
| for name, files in bundle.iteritems(): | ||
| # Set the paths to the files. | ||
| concatted_file = path(ftype, '%s-all.%s' % (name, ftype,)) | ||
| compressed_file = path(ftype, '%s-min.%s' % (name, ftype,)) | ||
| concatted_file = path(self.bundles_dir, ftype, | ||
| '%s-all.%s' % (name, ftype,)) | ||
| compressed_file = path(self.bundles_dir, ftype, | ||
| '%s-min.%s' % (name, ftype,)) | ||
|
|
||
| if not os.path.exists(path(self.bundles_dir, ftype)): | ||
| os.makedirs(path(self.bundles_dir, ftype)) | ||
|
|
||
| files_all = [] | ||
| for fn in files: | ||
| processed = self._preprocess_file(fn) | ||
| processed = self._preprocess_file(fn, compressed_file) | ||
| # If the file can't be processed, we skip it. | ||
| if processed is not None: | ||
| files_all.append(processed) | ||
|
|
@@ -111,6 +118,17 @@ def handle(self, **options): | |
| else: | ||
| self.minify_skipped += 1 | ||
|
|
||
| # Cleanup | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| print 'Clean up temporary files' | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this should probably be a comment instead. |
||
| for file in self.tmp_files: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. forgive my ignorance. is this different from
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it is this is getting rid of the temporary files that are being created for each one of the css files in which the URLs are corrected.
|
||
| try: | ||
| os.remove(path(file)) | ||
|
|
||
| # Try to remove the temporary directory (only removed if empty) | ||
| os.rmdir(os.path.dirname(path(file))) | ||
| except OSError: | ||
| pass | ||
|
|
||
| # Write out the hashes | ||
| self.update_hashes(options.get('add_timestamp', False)) | ||
|
|
||
|
|
@@ -134,11 +152,11 @@ def _get_url_or_path(self, item): | |
| """ | ||
| if item.startswith('//'): | ||
| return 'http:%s' % item | ||
| elif item.startswith(('http', 'https')): | ||
| elif item.startswith(('http:', 'https:')): | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good fix but seems unrelated |
||
| return item | ||
| return None | ||
|
|
||
| def _preprocess_file(self, filename): | ||
| def _preprocess_file(self, filename, compressed_file): | ||
| """Preprocess files and return new filenames.""" | ||
| url = self._get_url_or_path(filename) | ||
| if url: | ||
|
|
@@ -186,6 +204,12 @@ def _preprocess_file(self, filename): | |
| (settings.STYLUS_BIN, os.path.dirname(fp), fp, fp), | ||
| shell=True, stdout=PIPE) | ||
| filename = '%s.css' % filename | ||
|
|
||
| if not url: | ||
| # Fix relative paths | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. end comments with periods. |
||
| filename = self._fix_urls(filename, compressed_file) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this should be optional. on Marketplace at least, we don't want this to happen.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll add a setting in to control this. |
||
| self.tmp_files.append(filename) | ||
|
|
||
| return path(filename.lstrip('/')) | ||
|
|
||
| def _is_changed(self, concatted_file): | ||
|
|
@@ -205,6 +229,40 @@ def _clean_tmp(self, concatted_file): | |
| os.remove(concatted_file) | ||
| os.rename(tmp_concatted, concatted_file) | ||
|
|
||
| def _fix_urls(self, filename, compressed_file): | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. tests for this method would be great
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i'll add some |
||
| """Fix the relative URL's in css files""" | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "URL's" should be "URLs"
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what does "Fix" mean? can you reword the comment?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll reword the comment, but basically what it does is when bundling the file moves it from say |
||
| print "Fixing URL's in %s" % filename | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you sure you want to keep these
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was using them to debug and just never got rid of them. I'll ditch them. |
||
|
|
||
| css_content = '' | ||
| with open(path(filename)) as css_in: | ||
| css_content = css_in.read() | ||
|
|
||
| relpath = os.path.relpath(os.path.dirname(compressed_file), | ||
| path(os.path.dirname(filename))) | ||
|
|
||
| parse = lambda url: self._fix_urls_regex(url, relpath) | ||
|
|
||
| css_parsed = re.sub(r'url\(([^)]*?)\)', parse, css_content) | ||
|
|
||
| # Write to a temporary file | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. period. |
||
| out_file = path(os.path.join(self.bundles_dir, '.tmp', | ||
| '%s.tmp' % filename)) | ||
|
|
||
| if not os.path.exists(os.path.dirname(out_file)): | ||
| os.makedirs(os.path.dirname(out_file)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've always been told this should be written as |
||
|
|
||
| with open(out_file, 'w') as css_out: | ||
| css_out.write(css_parsed) | ||
|
|
||
| return os.path.relpath(out_file, path('.')) | ||
|
|
||
| def _fix_urls_regex(self, url, relpath): | ||
| """Run over the regex; Fix relative URL's""" | ||
| url = url.group(1).strip('"\'') | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doing
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the more I learn, very nice |
||
| if not url.startswith(('data:', 'http:', 'https:')): | ||
| url = os.path.relpath(url, relpath) | ||
| return 'url(%s)' % url | ||
|
|
||
| def _cachebust(self, css_file, bundle_name): | ||
| """Cache bust images. Return a new bundle hash.""" | ||
| print "Cache busting images in %s" % re.sub('.tmp$', '', css_file) | ||
|
|
@@ -268,7 +326,7 @@ def _file_hash(self, url): | |
| def _cachebust_regex(self, img, parent): | ||
| """Run over the regex; img is the structural regex object.""" | ||
| url = img.group(1).strip('"\'') | ||
| if url.startswith('data:') or url.startswith('http'): | ||
| if url.startswith(('data:', 'http:', 'https:')): | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh, nice catch |
||
| return "url(%s)" % url | ||
|
|
||
| url = url.split('?')[0] | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,5 @@ | ||
| import os | ||
| import tempfile | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add a newline below to separate the import blocks |
||
| from django.conf import settings | ||
| from django.test.utils import override_settings | ||
|
|
||
|
|
@@ -6,6 +8,7 @@ | |
| from nose.tools import eq_ | ||
|
|
||
| from .utils import get_media_root, get_media_url | ||
| from .management.commands.compress_assets import Command | ||
|
|
||
| try: | ||
| from build import BUILD_ID_CSS, BUILD_ID_JS | ||
|
|
@@ -66,8 +69,9 @@ def test_js_helper(getmtime, time): | |
| t = env.from_string("{{ js('common_protocol_less_url', debug=False) }}") | ||
| s = t.render() | ||
|
|
||
| eq_(s, '<script src="%sjs/common_protocol_less_url-min.js?build=%s">' | ||
| '</script>' % (settings.STATIC_URL, BUILD_ID_JS)) | ||
| eq_(s, | ||
| '<script src="%sjs/common_protocol_less_url-min.js?build=%s">' | ||
| '</script>' % (settings.STATIC_URL, BUILD_ID_JS)) | ||
|
|
||
| t = env.from_string("{{ js('common_bundle', debug=True) }}") | ||
| s = t.render() | ||
|
|
@@ -80,8 +84,8 @@ def test_js_helper(getmtime, time): | |
| t = env.from_string("{{ js('common_bundle', debug=False) }}") | ||
| s = t.render() | ||
|
|
||
| eq_(s, '<script src="%sjs/common_bundle-min.js?build=%s"></script>' % | ||
| (settings.STATIC_URL, BUILD_ID_JS)) | ||
| eq_(s, '<script src="%sjs/common_bundle-min.js?build=%s"></script>' | ||
| % (settings.STATIC_URL, BUILD_ID_JS)) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. typically people leave operators on the previous line. I forget where PEP draws the line. |
||
|
|
||
|
|
||
| @patch('jingo_minify.helpers.time.time') | ||
|
|
@@ -163,6 +167,33 @@ def test_css_helper(getmtime, time): | |
| (settings.STATIC_URL, BUILD_ID_CSS)) | ||
|
|
||
|
|
||
| @patch('jingo_minify.helpers.time.time') | ||
| @patch('jingo_minify.helpers.os.path.getmtime') | ||
| @override_settings(STATIC_ROOT='static', | ||
| MEDIA_ROOT='media', | ||
| STATIC_URL='http://example.com/static/', | ||
| MEDIA_URL='http://example.com/media/', | ||
| BUNDLES_DIR='bundled') | ||
| def test_bundles_dir(getmtime, time): | ||
| getmtime.return_value = 1 | ||
| time.return_value = 1 | ||
| env = jingo.env | ||
|
|
||
| t = env.from_string("{{ js('common', debug=False) }}") | ||
| s = t.render() | ||
|
|
||
| eq_(s, '<script src="%sbundled/js/common-min.js?build=%s"></script>' % | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. one line 88 you have the |
||
| (settings.STATIC_URL, BUILD_ID_JS)) | ||
|
|
||
| t = env.from_string("{{ css('common', debug=False) }}") | ||
| s = t.render() | ||
|
|
||
| eq_(s, | ||
| '<link rel="stylesheet" media="screen,projection,tv" ' | ||
| 'href="%sbundled/css/common-min.css?build=%s" />' | ||
| % (settings.STATIC_URL, BUILD_ID_CSS)) | ||
|
|
||
|
|
||
| def test_inline_css_helper(): | ||
| env = jingo.env | ||
| t = env.from_string("{{ inline_css('common', debug=True) }}") | ||
|
|
@@ -296,3 +327,78 @@ def test_js(getmtime, time): | |
| for j in settings.MINIFY_BUNDLES['js']['common']]) | ||
|
|
||
| eq_(s, expected) | ||
|
|
||
|
|
||
| @override_settings(STATIC_ROOT='static', | ||
| MEDIA_ROOT='media', | ||
| STATIC_URL='http://example.com/static/', | ||
| MEDIA_URL='http://example.com/media/') | ||
| def test_fix_urls(): | ||
| """Make sure relative URL's are fixed correctly""" | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "URL's" should be "URLs." |
||
| command = Command() | ||
|
|
||
| tmp = tempfile.mkstemp()[1] | ||
|
|
||
| tmp_file = open(tmp, 'w') | ||
| tmp_file.write('url(../img/test.png);') | ||
| tmp_file.close() | ||
|
|
||
| # Test files in same directory | ||
| compressed = command._fix_urls(tmp, | ||
| os.path.join(os.path.dirname(tmp), 'compiled')) | ||
| compressed_file = open(compressed, 'r') | ||
| eq_(compressed_file.read(), 'url(../img/test.png);') | ||
| compressed_file.close() | ||
|
|
||
| # Test files in different directory | ||
| compressed = command._fix_urls(tmp, | ||
| os.path.join(os.path.dirname(tmp), 'dir', 'compiled')) | ||
| compressed_file = open(compressed, 'r') | ||
| eq_(compressed_file.read(), 'url(../../img/test.png);') | ||
| compressed_file.close() | ||
|
|
||
| # Remove temporary files | ||
| os.remove(compressed) | ||
| os.remove(tmp) | ||
|
|
||
| # Test data urls are unchanged | ||
| data_url = ('url(data:image/gif;base64,R0lGODlhEAAQAMQAAORHHOVSKudfOulr' | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does this need to be an actually valid data URI?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nope, i can ditch the full thing and make it something smaller. |
||
| 'SOp3WOyDZu6QdvCchPGolfO0o/XBs/fNwfjZ0frl3/zy7////wAAAAAAAA' | ||
| 'AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAACH5' | ||
| 'BAkAABAALAAAAAAQABAAAAVVICSOZGlCQAosJ6mu7fiyZeKqNKToQGDsM8' | ||
| 'hBADgUXoGAiqhSvp5QAnQKGIgUhwFUYLCVDFCrKUE1lBavAViFIDlTImbK' | ||
| 'C5Gm2hB0SlBCBMQiB0UjIQA7);') | ||
|
|
||
| tmp = tempfile.mkstemp()[1] | ||
| tmp_file = open(tmp, 'w') | ||
| tmp_file.write(data_url) | ||
| tmp_file.close() | ||
|
|
||
| compressed = command._fix_urls(tmp, | ||
| os.path.join(os.path.dirname(tmp), 'compiled')) | ||
| compressed_file = open(compressed, 'r') | ||
| eq_(compressed_file.read(), data_url) | ||
| compressed_file.close() | ||
|
|
||
| # Remove temporary files | ||
| os.remove(compressed) | ||
| os.remove(tmp) | ||
|
|
||
| # Test absolute paths are unchanged | ||
| content = ('url(http://example.com/image.gif); ' | ||
| 'url(https://example.com/image.png);') | ||
| tmp = tempfile.mkstemp()[1] | ||
| tmp_file = open(tmp, 'w') | ||
| tmp_file.write(content) | ||
| tmp_file.close() | ||
|
|
||
| compressed = command._fix_urls(tmp, | ||
| os.path.join(os.path.dirname(tmp), 'compiled')) | ||
| compressed_file = open(compressed, 'r') | ||
| eq_(compressed_file.read(), content) | ||
| compressed_file.close() | ||
|
|
||
| # Remove temporary files | ||
| os.remove(compressed) | ||
| os.remove(tmp) | ||
|
|
||
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,3 +2,4 @@ Django==1.4.5 | |
| jingo==0.4 | ||
| -e git://github.com/jbalogh/django-nose.git@83c7867c3f90ff3c7c7471716da91b643e8b2c01#egg=django_nose-dev | ||
| mock==1.0b1 | ||
| GitPython==0.1.7 | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can't find where you use that package in the diff.
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's unrelated but missing. There's a test in #63 that demonstrates the issue. Any new tests that hit that area of code will trip over it.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why the added dependency? I don't see where it's used
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Issue #26 |
||
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 would remove line 73, and just wrap this and put the string below on line 75.