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

refactor: drop jinja2-ansible-filters and reimplement Jinja2 filters #1809

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sisp
Copy link
Member

@sisp sisp commented Oct 10, 2024

I've dropped the jinja2-ansible-filters dependency and reimplemented most of the Jinja2 filters by relying on the Ansible filters documentation as much as possible to avoid copying/rewriting GPL-licensed code. The tests are based on a combination of Ansible filters documentation and some thinking of my own.

This is the current status:

  • ans_groupby
  • ans_random
  • b64decode
  • b64encode
  • basename
  • bool
  • checksum
  • comment – This filter seems a bit involved, and I'm not sure how relevant it is for Copier.
  • dirname
  • expanduser
  • expandvars
  • extract
  • fileglob
  • flatten
  • from_json
  • from_yaml
  • from_yaml_all
  • hash
  • mandatory
  • md5
  • quote
  • random_mac
  • realpath
  • regex_escape – Only re_type = "python" is implemented, not re_type = "posix_basic" because I'm uncertain how to implement it without relying on the GPL-licensed implementation.
  • regex_findall
  • regex_replace
  • regex_search
  • relpath
  • sha1
  • shuffle
  • splitext
  • strftime
  • subelements – This filter seems a bit involved, and I'm not sure how relevant it is for Copier.
  • ternary
  • to_datetime
  • to_json
  • to_nice_json
  • to_nice_yaml
  • to_uuid
  • to_yaml
  • type_debug
  • win_basename
  • win_dirname
  • win_splitdrive

At this point, 2 filters are missing and one is only partially implemented. This means, if we decide to merge this PR without adding the missing implementations, it would be a breaking change (besides some residual risk of a bad reimplementation).

I'm marking this PR as a draft for now to ensure we properly assess its state before merging.

WDYT, @copier-org/maintainers?

Fixes #1398.

@sisp sisp marked this pull request as draft October 10, 2024 15:48
@sisp sisp force-pushed the refactor/drop-jinja2-ansible-filters branch from b831a07 to 795ee70 Compare October 10, 2024 16:22
Copy link

codecov bot commented Oct 10, 2024

Codecov Report

Attention: Patch coverage is 95.40816% with 9 lines in your changes missing coverage. Please review.

Project coverage is 97.54%. Comparing base (a4d38b1) to head (0480ab4).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
copier/jinja_ext.py 95.18% 8 Missing ⚠️
tests/test_jinja2_extensions.py 96.42% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1809      +/-   ##
==========================================
- Coverage   97.67%   97.54%   -0.13%     
==========================================
  Files          49       50       +1     
  Lines        5108     5302     +194     
==========================================
+ Hits         4989     5172     +183     
- Misses        119      130      +11     
Flag Coverage Δ
unittests 97.54% <95.40%> (-0.13%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sisp sisp force-pushed the refactor/drop-jinja2-ansible-filters branch 7 times, most recently from c45cf12 to d72bd50 Compare October 11, 2024 14:29
@sisp sisp force-pushed the refactor/drop-jinja2-ansible-filters branch from d72bd50 to 0480ab4 Compare October 11, 2024 14:43
@yajo
Copy link
Member

yajo commented Oct 12, 2024

OMG you went so far with this! 😅

I thought just to_yaml would be enough for us.

But well, now that's done, I IMHO think that the best you could do is release them as a separate library. These filters are very useful under many contexts outside of copier. Then we can just swap our default dependency here.

@sisp
Copy link
Member Author

sisp commented Oct 12, 2024

OMG you went so far with this! 😅

I thought just to_yaml would be enough for us.

Well, I was aiming for a drop-in replacement. In fact, I've bee using some regex filters and ternary myself. But those two missing filters and the missing variant for regex_escape are causing some headaches.

But well, now that's done, I IMHO think that the best you could do is release them as a separate library. These filters are very useful under many contexts outside of copier. Then we can just swap our default dependency here.

I can definitely create a separate library. Should we create a project in the copier-org organization? And how would we name the package? Some ideas:

  • copier-jinja2-extension
  • jinja2-copier-extension
  • jinja2-copier-filters – Similar to jinja2-ansible-filters. Naming indicates filters; what happens if we add tests?
  • A more descriptive name? But it's a mix of filters, not sure what a good name would be.

I slightly dislike the Ansible-related naming of the ans_* prefixed filters. If we went for a truly separate library without aiming for a drop-in replacement of jinja2-ansible-filters, I'd prefer using generic filter names. But if we merely depend on this library and load the Jinja extension as is, then we'll need to make a major Copier release because of the non-backwards compatible change.

@pawamoy
Copy link
Contributor

pawamoy commented Oct 12, 2024

@sisp you can always support ans_* filters in the separate library for some amount of time, while deprecating them in favor of equivalent, renamed filters, so that downstream Copier users get the warnings and update their templates. Then the major breaking change only happens in this external lib and Copier doesn't have to do anything particular, except maybe accentuate the communication around this change.

@sisp
Copy link
Member Author

sisp commented Oct 12, 2024

@pawamoy But since Copier exposes those Jinja filters as default/builtin filters, isn't a breaking change in those filters also a breaking change in Copier? 🤔 The situation would be different if we only offered the library and advertised its use via the _jinja_extensions setting; then, template authors would be responsible for maintaining compatibility and documenting the supported version specifier for our library.

@pawamoy
Copy link
Contributor

pawamoy commented Oct 12, 2024

Yeah I'm not sure how maintainers usually deal with breaking changes in dependencies other than patching and owning support themselves, then do a major bump 🤔

I guess what I meant is that it's probably easier to handle it in the external lib rather in Copier. Copier can then wash its hands, relying on the fact that the external lib emitted all due warnings. Probably.

@yajo
Copy link
Member

yajo commented Oct 13, 2024

I think we should make a breaking change anyway. There is no guarantee that nothing will break.

The only reason we need to bundle some filters by default is that we recommend to_nice_yaml in the docs. And the only reason it's done is because today I just realized I've been misunderstanding the docs about Jinja's built-in tojson filter all my life. The docs say it's safe to render in HTML docs, so I always understood it would dump HTML escape characters here and there, where it turns out it just uses JSON escape characters, valid in any other context.

So, we could just change our docs and recommend using that filter instead. Then remove the dependency and only suggest it.

This would be the easiest approach. However, the impact would be huge. 99% of templates would be broken (scientific research citation missing 😆). Many people don't know that YAML is a superset of JSON, so they would wonder why we're using JSON in a YAML document. And while the copier answers file is supposedly not hand-edited, it would still become much uglier.

In hindsight, I should have used .copier-answers.json with the tojson filter from the beginning. 🤷‍♂️

Now, back to the reality, let's create a new Copier project + repo !

  • jinja2-copier-extension

I guess this is good enough.

@yajo
Copy link
Member

yajo commented Oct 13, 2024

https://github.com/copier-org/jinja2-copier-extension

@sisp is the admin there. 😉

@sisp
Copy link
Member Author

sisp commented Oct 13, 2024

Yeah I'm not sure how maintainers usually deal with breaking changes in dependencies other than patching and owning support themselves, then do a major bump 🤔

@pawamoy Typically, some functionality of a dependency is used internally where it isn't directly exposed to the users and we merely use it to solve a problem in our software. For instance, when we migrated from Pydantic v1 to v2, we needed to adapt our use of Pydantic, but the change was transparent to the users because we compensated for the breaking changes in Pydantic in our internal code. I think it's different when we depend on a library and pass through its functionality as if it was bundled with Copier. In that case, breaking changes also get passed through, so they affect Copier directly. Perhaps pinning the library's version is the right way to handle this case. Then, we are in control of what version of the library is used and when it gets updated, so it's very similar to vendoring the library or inlining its code in our code base.

However, the impact would be huge. 99% of templates would be broken (scientific research citation missing 😆).

@yajo Yes, even omitting some other filters from jinja2-ansible-filters would probably break many templates. This would likely be very harmful to adoption of and trust in Copier.

Thanks for creating the jinja2-copier-extension project. I'll move the filters there, hopefully some time next week.

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.

pyyaml-include is GPL3, doesn't that poison your MIT project?
3 participants