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

Import Markup and escape directly from MarkupsSafe #1737

Merged
merged 5 commits into from Mar 28, 2022

Conversation

rwedge
Copy link
Contributor

@rwedge rwedge commented Mar 24, 2022

Fixes #1736

Jinja 3.1.0 removed jinja.Markup and jinja.escape and suggested importing them from MarkupSafe

mauicv added a commit to SeldonIO/alibi-detect that referenced this pull request Mar 25, 2022
Revert once jupyter/nbconvert#1737 is merged and released!
@rwedge
Copy link
Contributor Author

rwedge commented Mar 25, 2022

Test failure appears related to Jinja2 3.1.0 release. Pin Jinja2 below 3.1.0 and the tests pass.

Possibly related to this change? pallets/jinja#1621

@trungleduc
Copy link
Contributor

should we bump the lower bound and add an upper bound for jinja2 version?

@rwedge
Copy link
Contributor Author

rwedge commented Mar 25, 2022

I set an upper bound for jinja2. Perhaps a different PR could investigate the windows errors?

@rwedge rwedge requested a review from minrk March 25, 2022 15:46
@effigies
Copy link

This also closes #1605.

@rwedge
Copy link
Contributor Author

rwedge commented Mar 25, 2022

@davidism could there have been a change in jinja2 behavior between 3.0 and 3.1 that would account for the new failures in these tests?

    def test_absolute_template_dir(self):
        with tempdir.TemporaryDirectory() as td:
            template = 'mytemplate'
            template_file = os.path.join(td, template, 'index.py.j2')
            template_dir = os.path.dirname(template_file)
            os.mkdir(template_dir)
            test_output = 'absolute!'
            with open(template_file, 'w') as f:
                f.write(test_output)
            config = Config()
            config.TemplateExporter.template_name = template
            config.TemplateExporter.extra_template_basedirs = [td]
            exporter = self._make_exporter(config=config)
>           assert exporter.template.filename == template_file
E           AssertionError: assert 'C:\\Users\\R...e/index.py.j2' == 'C:\\Users\\R...\\index.py.j2'
E             - C:\Users\RUNNER~1\AppData\Local\Temp\tmpd72mcthi\mytemplate\index.py.j2
E             ?                                                            ^
E             + C:\Users\RUNNER~1\AppData\Local\Temp\tmpd72mcthi\mytemplate/index.py.j2
E             ?     

https://github.com/jupyter/nbconvert/runs/5689650683?check_suite_focus=true

@davidism
Copy link

davidism commented Mar 25, 2022

We did fix some issues with unsafe Windows paths in template names. The two filenames being compared are in fact equivalent on Windows, even though they're not textually equivalent, since / is a valid path separator. A quick workaround for you would be to use pathlib.Path() or os.path.normpath() on both values, which will normalize separators, which is what I'll do in a bugfix for Jinja as well.

@davidism
Copy link

Created pallets/jinja#1637, I will submit a PR and then release 3.1.1.

@davidism
Copy link

Released Jinja2 3.1.1 with the filename fix.

@SylvainCorlay
Copy link
Member

Thanks everyone.

@SylvainCorlay SylvainCorlay merged commit c91a038 into jupyter:main Mar 28, 2022
@SylvainCorlay
Copy link
Member

I wonder if we should exclude 3.1.0 (while accepting 3.1.1) from the versions in setup.py.

matthewfeickert added a commit to scikit-hep/pyhf that referenced this pull request Apr 3, 2022
* Revert PR #1824 to remove restrictions place on Jinja2 given
spatialaudio/nbsphinx#641. The issues
described there were resolved upstream of nbsphinx in
jupyter/nbconvert#1737.
   - Lower bounds of 'nbconvert>=6.4.5' are not added as lower
     bound restrictions should be applied upstream of pyhf in
     nbsphinx. PR #1824 was applied only because it was
     necessary.
douglatornell added a commit to rhwhite/numeric_2022 that referenced this pull request Apr 5, 2022
nbconvert has been updated re: jinja=3.10.
See spatialaudio/nbsphinx#641 (comment)
and jupyter/nbconvert#1737
douglatornell added a commit to rhwhite/numeric_2022 that referenced this pull request Apr 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] module 'jinja2.utils' has no attribute 'escape'
6 participants