tera
tera copied to clipboard
Keats
Isolate chrono use from rest of "builtins" features
The chrono dependency is not actively being maintained, and there's known issues with the older version of the time lib it depends on. Since not all uses of Tera requires the date filter support, this PR moves all chrono related definitions, use and tests under the chrono feature. This allows other builtins features to be used, while not bringing in the chrono dependency.
https://github.com/Keats/tera/issues/706 https://www.reddit.com/r/rust/comments/qamgyh/is_the_chrono_crate_unmaintained/
But the default inclusion is the same. It's only relevant if one wants to omit using the chrono dependency, like we want in pg_datanymizer. Without this, we have to drop all builtins which reduces functionality needlessly.
Do you have any other suggestion for how we can omit just chrono and not have to maintain a fork of Tera?
I could leave builtins the same and make a feature alias for the subset "builtins-less-chrono".
@Keats I just updated the feature lists to make the default and "builtins" the same as before, so no breaking change.
There is now a feature builtins-less-chrono to avoid using the unmaintained chrono library, so only those that ask for it will see the difference.
The idea is good but I think you need to remove some of the changes from your PR, there are no chrono features so only the changes in Cargo.toml are needed I think.
There's a problem with the suggested approach. In short, I don't think it can be done all in Cargo.toml. If I'm wrong, please help me understand how it can be done and I'll do it. Otherwise, I'll just have to maintain a fork of Tera, which I'd really like to avoid.
"builtins" defines several feature labels (e.g., "slug", "percent-encoding", "humansize", "rand", "chrono", "chrono-tz") that causes other deps to be pulled in. All Tera functions that use any of these are included with the "builtins" feature:
- now() which uses "chrono" (link)
- get_random() which uses "rand"
If I wanted to remove the "chrono" dependency but keep "rand" and get_random(), I am stuck. They are all labeled with "builtins" so it's an all or none.
The only way I can see to make it work is label all the dependent uses and functions for "chrono" with "chrono", for "rand" with "rand", etc, and add a catch-all sub-feature called "builtins-core" (for example) for all the other-builtins and label all those in the code. Nothing in the code is labeled with the "builtins" feature directly. Then "builtins" is just a group of other features that can be included or not.
[features]
default = ["builtins"]
builtins = ["builtins-core", "slug", "percent-encoding", "humansize", "rand", "chrono", "chrono-tz"]
builtins-less-chrono = ["builtins-core", "slug", "percent-encoding", "humansize", "rand"]
builtins-less-rand = ["builtins-core", "slug", "percent-encoding", "humansize", "chrono", "chrono-tz"]
Let me know what you think @Keats before I attempt it.
Thank you!
Man, for the next version each dependency is getting its own feature.
My comment was more about that in the changes in this PR use the #[cfg(feature = "chrono")] which doesn't exist but I just remember the implicit feature for optional deps >_>
I believe your PR is fine, can you add a test https://github.com/Keats/tera/blob/master/.github/workflows/ci.yml#L43-L44 for the builtins-less-chrono feature?
Man, for the next version each dependency is getting its own feature.
That would actually be cool - but also kind of the opposite of what this PR is doing? If I understand it correctly, this PR allows to exclude one dependency, and keeping all others, instead of allowing the user to pick exactly the ones they need.
That would actually be cool - but also kind of the opposite of what this PR is doing? I
Not that different. There could still be a list of built-in features. Each filter/test/fn requiring a new dependency would get a feature and this way users can tailor their dependencies so the end result is the same as this PR roughly.
slight tanget but, for what its worth my projects explicitly ban chrono due to security and maintainership issues. other crates i use swapped to time directly with no loss of function. instead of just splitting this off, what about swapping it out like other crates for time itself? two birds (maintainership and security), one stone
similar to https://github.com/Keats/tera/issues/706
not sure, but this pr https://github.com/Drakulix/simplelog.rs/pull/95 might help @Keats it isnt the only one that switched but only one i could think off off hand
I've already replaced chrono with time in a few projects, it's just that Tera uses strftime so unless there is a crate for that with time, I'm not going to write my own implementation.
There appears to be a crate that brings strfitme-compatible formatting to time: https://crates.io/crates/time-fmt
Maybe tera could use this crate to replace chrono?
Hm it might work! I (or someone else if they have time) should check how easy it is to replace the current chrono usage with time + time-fmt