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 enhancements #2733

Merged
merged 7 commits into from
Aug 15, 2018
Merged

Conversation

TomekTrzeciak
Copy link
Contributor

@TomekTrzeciak TomekTrzeciak commented Jul 23, 2018

The future plan is to have proper API for Cylc, but this future is still quite far away. In the meantime, we could have some improvements to make the templating experience better. This PR adds two such improvements:

  1. Support for Jinja2Tests and Jinja2Globals to complement the existing Jinja2Filters (thus enabling custom additions to all three namespaces used in Jinja2). This is useful as some Jinja2 functionality is awkward to use without them, e.g., select/reject filters, which use the tests namespace.
  2. Support for importing Python modules in the same way as the template modules. This is useful, because the default Jinja2 functionality is fairly bare bones and there are some glaring omissions, like no Python zip equivalent, which makes some basic programming tasks quite awkward. Rather than keep adding useful bits of functionality one-by-one, it is left up to the user to decide which packages are of the most use.

TODO:

  • CUG documentation (I didn't want to spend time on it before hearing some feedback first)

@hjoliver
Copy link
Member

This definitely sounds useful, no objections from me. A couple of examples would be good, so I don't need to work it out (short of time!) - presumably your CUG documentation will add that.

@matthewrmshin
Copy link
Contributor

Some test failures here. Should be easy enough to fix.

@hjoliver
Copy link
Member

(Regarding my request for an example - specifically "loading Python modules in Jinja2 templates"; the other bits are reasonably obvious to me).

@TomekTrzeciak
Copy link
Contributor Author

Added docs and rebased. @hjoliver, for an example see the new section "Importing additional Python modules" in the docs.

@matthewrmshin
Copy link
Contributor

Codacy is hard to please, but it may have a point. Do you think this one is easy to solve?

@TomekTrzeciak
Copy link
Contributor Author

@matthewrmshin, in this case it's impossible to please codacy. It gives me "Parameters differ from overridden 'load' method", but the parameter in question is named globals and using that triggers another complain about overriding the built-in function.

@TomekTrzeciak
Copy link
Contributor Author

I believe this PR is ready for review now.

@TomekTrzeciak
Copy link
Contributor Author

Codacy drives me nuts 😡 I've fixed the complaints but this triggered hotspot detection due to frequent changes. Agrhhh...
Hopefully this will go away after the merge.

@hjoliver
Copy link
Member

Codacy is useful, but yeah, it can be maddening. We don't require a complete "pass" when it's obviously full of 💩

@matthewrmshin
Copy link
Contributor

(Agree that we should just ignore Codacy for this change.)

Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

Looks good to me; nicely documented; python module import will be super useful. (One trivial spelling error).

@TomekTrzeciak
Copy link
Contributor Author

Fix the typo and codacy seems to be happy this time around 😁

@hjoliver
Copy link
Member

@TomekTrzeciak - the user guide changes here now conflict slightly with your EmPy support (just merged). Should be easy to resolve by accepting both changes and checking placement, I expect.

@matthewrmshin
Copy link
Contributor

(Branch in small conflict.)

@TomekTrzeciak
Copy link
Contributor Author

Fixed, should merge cleanly now.

@matthewrmshin matthewrmshin merged commit 6ba43fd into cylc:master Aug 15, 2018
@TomekTrzeciak TomekTrzeciak mentioned this pull request Aug 15, 2018
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.

3 participants