copier icon indicating copy to clipboard operation
copier copied to clipboard

refactor: drop `jinja2-ansible-filters` and reimplement Jinja2 filters

Open sisp opened this issue 1 year ago • 9 comments

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:

  • [x] ans_groupby
  • [x] ans_random
  • [x] b64decode
  • [x] b64encode
  • [x] basename
  • [x] bool
  • [x] checksum
  • [ ] comment – This filter seems a bit involved, and I'm not sure how relevant it is for Copier.
  • [x] dirname
  • [x] expanduser
  • [x] expandvars
  • [x] extract
  • [x] fileglob
  • [x] flatten
  • [x] from_json
  • [x] from_yaml
  • [x] from_yaml_all
  • [x] hash
  • [x] mandatory
  • [x] md5
  • [x] quote
  • [x] random_mac
  • [x] realpath
  • [x] 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.
  • [x] regex_findall
  • [x] regex_replace
  • [x] regex_search
  • [x] relpath
  • [x] sha1
  • [x] shuffle
  • [x] splitext
  • [x] strftime
  • [ ] subelements – This filter seems a bit involved, and I'm not sure how relevant it is for Copier.
  • [x] ternary
  • [x] to_datetime
  • [x] to_json
  • [x] to_nice_json
  • [x] to_nice_yaml
  • [x] to_uuid
  • [x] to_yaml
  • [x] type_debug
  • [x] win_basename
  • [x] win_dirname
  • [x] 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 avatar Oct 10 '24 15:10 sisp

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 :warning:
tests/test_jinja2_extensions.py 96.42% 1 Missing :warning:
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%) :arrow_down:

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Oct 10 '24 16:10 codecov[bot]

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.

yajo avatar Oct 12 '24 13:10 yajo

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.

sisp avatar Oct 12 '24 13:10 sisp

@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.

pawamoy avatar Oct 12 '24 13:10 pawamoy

@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? :thinking: 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.

sisp avatar Oct 12 '24 18:10 sisp

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.

pawamoy avatar Oct 12 '24 22:10 pawamoy

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 avatar Oct 13 '24 04:10 yajo

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

@sisp is the admin there. 😉

yajo avatar Oct 13 '24 04:10 yajo

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.

sisp avatar Oct 13 '24 07:10 sisp