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

[BUG: 1401246] Update jinja and django-jinja #4747

Merged
merged 2 commits into from Apr 24, 2018

Conversation

Projects
None yet
4 participants
@Alig1493
Contributor

Alig1493 commented Apr 23, 2018

According to the Jinja2 v2.9 changelog:
The with and autoescape tags are now built-in.

Thus I have removed them from the settings file.

Link: http://jinja.pocoo.org/docs/2.10/changelog/#version-2-9

@Alig1493 Alig1493 changed the title from Update jinja to [BUG: 1401246] Update jinja and django-jinja Apr 23, 2018

@@ -50,7 +50,7 @@ def test_message_with_url_is_link(self):
m = Message(message='Go to http://bit.ly/sample-demo', is_global=True,
is_active=True, url='/')
m.save()
ok_('Go to <a href="http://bit.ly/sample-demo">'
ok_('Go to <a href="http://bit.ly/sample-demo" rel="noopener">'

This comment has been minimized.

@safwanrahman

safwanrahman Apr 23, 2018

Contributor

According to jinja2 policy, seems like noopener is default

@jwhitlock

@Alig1493, thanks for this update, and thanks for using the new standards for requirements files - this made it easier to review.

I'm going to make a couple of changes:

  • The bug for requirement updates is bug 1399639
  • I'm going to change the way the locale build is done in TravisCI so the build will pass, like it does locally.
--hash=sha256:a4ec1aff59b95a14b45eb2e23761a0179e98319da5a7eb76b56ea8cdc7b871c3
# Used for Jinja2
# Code: https://github.com/pallets/markupsafe
# Changes: https://github.com/pallets/markupsafe/blob/master/CHANGES

This comment has been minimized.

@jwhitlock

jwhitlock Apr 24, 2018

Member

👍 thanks for adding the project details. It appears the jump to from 0.23 to 1.0 is about the project being feature-complete, and dropping Python 2.6 support.

--hash=sha256:04c53ba90cbede97e6543b98291f1eec259e57dfa4e014a5fbf0291afcdae6a9
# Code: https://github.com/niwinz/django-jinja
# Changes: https://github.com/niwinz/django-jinja/blob/master/CHANGES.adoc
# Docs: http://niwinz.github.io/django-jinja/latest/

This comment has been minimized.

@jwhitlock

jwhitlock Apr 24, 2018

Member

Again, thanks for the project details. 2.3.0 added Django 1.11 compatibility, along with a reverted experiment in loading Jinja2 templates inside Django templates.

--hash=sha256:1cc03ef32b64be19e0a5b54578dd790906a34943fe9102cfdae0d4495bd536b4 \
--hash=sha256:bc1ff2ff88dbfacefde4ddde471d1417d3b304e8df103a7a9437d47269201bf4
# Code: https://github.com/pallets/jinja
# Changes: http://jinja.pocoo.org/docs/2.10/changelog/

This comment has been minimized.

@jwhitlock

jwhitlock Apr 24, 2018

Member

Several changes that will impact Kuma

  • urlize now sets “rel noopener” by default.
  • with and autoescape tags are built-in
  • Added a (default on?) trimmed modifier to {% trans %}
@@ -622,9 +622,7 @@ def _get_locales():
'extensions': [
'jinja2.ext.do',
'jinja2.ext.loopcontrols',
'jinja2.ext.with_',

This comment has been minimized.

@jwhitlock

jwhitlock Apr 24, 2018

Member

In TravisCI, the locales are built with the current Docker image, which means it is breaking on this change that requires the updated requirements are used. I'm going to add a new commit to change the locale build to act more like the py27, so the updated requirements will be used.

@codecov-io

This comment has been minimized.

codecov-io commented Apr 24, 2018

Codecov Report

Merging #4747 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4747      +/-   ##
==========================================
+ Coverage   95.83%   95.84%   +<.01%     
==========================================
  Files         270      270              
  Lines       24543    24625      +82     
  Branches     1740     1746       +6     
==========================================
+ Hits        23521    23601      +80     
- Misses        813      814       +1     
- Partials      209      210       +1
Impacted Files Coverage Δ
kuma/core/tests/test_helpers.py 100% <100%> (ø) ⬆️
kuma/wiki/tests/test_views_code.py 100% <0%> (ø) ⬆️
kuma/core/tests/test_middleware.py 100% <0%> (ø) ⬆️
kuma/attachments/tests/test_views.py 100% <0%> (ø) ⬆️
kuma/settings/common.py 92.18% <0%> (ø) ⬆️
kuma/wiki/tests/test_views_document.py 99.49% <0%> (+0.02%) ⬆️
kuma/core/middleware.py 88.28% <0%> (+1.15%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 78ddba4...520472b. Read the comment docs.

Alig1493 and others added some commits Apr 22, 2018

bug 1399639: Update to django-jinja 2.4.1, Jinja2
* django-jinja 2.2.1 → 2.4.1: Django 1.11 compatibility
* Jinja2 2.8 → 2.10: Default policy of "rel=nopener", trimmed trans
* MarkupSafe 0.23 → 1.0: Drop Python 2.6 support

Update configuration, and tests for rel=nopener.
bug 957802: Test new requirements in locale build
In the TravisCI locale build, use the Python environment and install any
new requirements, rather than using the current Docker image and the old
requirements. This allows verification of updates for Jinja2 and related
libraries.
@jwhitlock

This comment has been minimized.

Member

jwhitlock commented Apr 24, 2018

TravisCI passes, and I've verified that the extracted strings are the same, except for some line numbers. Thanks again @Alig1493!

@jwhitlock jwhitlock merged commit ad11381 into mozilla:master Apr 24, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Alig1493

This comment has been minimized.

Contributor

Alig1493 commented Apr 24, 2018

Thanks a lot for your fast and insightful review and for merging the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment