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

Jinja2 v3 #140

Merged
merged 2 commits into from
Jun 8, 2022
Merged

Jinja2 v3 #140

merged 2 commits into from
Jun 8, 2022

Conversation

oliver-sanders
Copy link
Member

@oliver-sanders oliver-sanders commented Apr 29, 2022

Sibling to an upcoming cylc-flow PR switching from Jinja2 v2->v3.

Jinja2v2 used to support zero-prefixed integers e.g. 01, 02, 03, ...

Jinja2v3 dropped support for this, fair enough, however, our users haven't been getting warnings for this so haven't had time to adapt.

This PR patches the Jinja2 parser we use in cylc-rose to shave off the leading zero before we pass the template variable back through to Jinja2 itself. It's not a particularly nice solution, it is intended as a stopgap to help users to migrate with the intention that we would retire it ASAP, probably with the move to Jinja2 3.1.

Requirements check-list

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Applied any dependency changes to setup.cfg.
  • Appropriate tests are included (unit and/or functional).
  • Appropriate change log entry included.
  • No documentation update required.
  • Made Discourse post to raise awareness of this change - https://cylc.discourse.group/t/jinja2-upgrade-to-version-3/489
    (note post not yet public, waiting for the rc4 release first)

@oliver-sanders
Copy link
Member Author

(note the tests will fail until cylc-flow moves to Jinja2 v3)

@oliver-sanders
Copy link
Member Author

oliver-sanders commented May 10, 2022

ATM this PR supports a single leading zero e.g. 01, 02, 03, ...

Looks like there is some usage of multiple leading zeros e.g. 001, 0002, 00003, ...

This diff should expand support to the latter cases:

diff --git a/cylc/rose/jinja2_parser.py b/cylc/rose/jinja2_parser.py
index 96cebae..9f95567 100644
--- a/cylc/rose/jinja2_parser.py
+++ b/cylc/rose/jinja2_parser.py
@@ -50,8 +50,11 @@ def _lexer_wrap(fcn):
                 and len(value_str) > 1
                 and value_str[0] == '0'
             ):
+                for ind, char in enumerate(value_str):
+                    if char != '0':
+                        break
                 instances.add(value_str)
-                value_str = value_str[1:]  # strip the leading zero
+                value_str = value_str[ind:]  # strip the leading zero
             yield (lineno, token, value_str)
 
     def _inner(

@MetRonnie
Copy link
Member

That diff does the trick 👍

However, noticed the warning you get should be

 WARNING - Support for integers with leading zeros was dropped in Jinja2 v3. Rose will extend support until a future version.
     Please amend your Rose configuration files e.g:
      * 0500 => 500
      * 050 => 50
      * 0900 => 900
-     * 0001 => 001
+     * 0001 => 1
      * 0100 => 100

@oliver-sanders
Copy link
Member Author

oliver-sanders commented May 10, 2022

FYI tested zero patted integers with Jinja2 v2 under Python 2.7 and 3.7 to make sure they behaved the same which they did:

$ cat suite.rc
#!Jinja2

{% set number = 007 %}

# {{ number }}
# {{ number + 2 }}
# {{ number + 002 }}

{% set number = 0099 %}

# {{ number }}
# {{ number + 2 }}
# {{ number + 002 }}

$ cylc view . --stdout --process
# 7
# 9
# 9
# 99
# 101
# 101

(no octal wierdness)

Jinja2 v3 octals take the same format as Python 3 octals.

Adding the patch (with a fix for the warnings)

@oliver-sanders oliver-sanders force-pushed the jinja2-v3 branch 2 times, most recently from 2e6e3fc to 09f105d Compare May 10, 2022 16:20
@hjoliver hjoliver added this to the 1.x milestone May 18, 2022
@MetRonnie MetRonnie self-requested a review June 1, 2022 09:38
@MetRonnie MetRonnie modified the milestones: 1.x, 1.1.0 Jun 1, 2022
@MetRonnie MetRonnie requested a review from wxtim June 1, 2022 10:59
cylc/rose/jinja2_parser.py Show resolved Hide resolved
cylc/rose/jinja2_parser.py Outdated Show resolved Hide resolved
cylc/rose/jinja2_parser.py Outdated Show resolved Hide resolved
Jinja2v2 used to support zero-prefixed integers e.g. 01, 02, 03, ...

Jinja2v3 dropped support for this, fair enough, however, our users
haven't been getting warnings for this so haven't had time to adapt.

This PR patches the Jinja2 parser we use in cylc-rose to shave off the
leading zero before we pass the template variable back through to Jinja2
itself. It's not a particularly nice solution, it is intended as a
stopgap to help users to migrate with the intention that we would retire
it ASAP, probably with the move to Jinja2 3.1.
@oliver-sanders oliver-sanders marked this pull request as ready for review June 7, 2022 14:49
CHANGES.md Outdated Show resolved Hide resolved
Co-authored-by: Ronnie Dutta <[email protected]>
Comment on lines +8 to +10
Support integers with leading zeros (e.g `001`) to back support Rose
configurations for use with cylc-flow>=8.0rc4 which uses Jinja2 v3 which
no longer supports this.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also this sentence is a bit difficult to parse, I would suggest something like

Suggested change
Support integers with leading zeros (e.g `001`) to back support Rose
configurations for use with cylc-flow>=8.0rc4 which uses Jinja2 v3 which
no longer supports this.
(For use with cylc-flow>=8.0rc4 which uses Jinja2 v3)
Maintain support for integers with leading zeros (e.g `001`)
in Rose configurations. Note this support is deprecated and you
should remove leading zeros.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that 4 lines is too many for CHANGES.md

@wxtim wxtim merged commit 994f33e into cylc:master Jun 8, 2022
@oliver-sanders oliver-sanders deleted the jinja2-v3 branch June 8, 2022 12:46
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.

4 participants