-
Notifications
You must be signed in to change notification settings - Fork 291
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
Isolate chrono use from rest of "builtins" features #711
base: master
Are you sure you want to change the base?
Isolate chrono use from rest of "builtins" features #711
Conversation
@Keats - what do you think of this change? |
That would be a breaking change I believe so it's not going to happen for v1 |
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 |
The idea is good but I think you need to remove some of the changes from your PR, there are no |
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:
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.
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. |
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. |
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 similar to #706 |
Is there a crate doing strftime handling for |
not sure, but this pr Drakulix/simplelog.rs#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 |
There appears to be a crate that brings 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 |
The
chrono
dependency is not actively being maintained, and there's known issues with the older version of thetime
lib it depends on. Since not all uses of Tera requires the date filter support, this PR moves allchrono
related definitions, use and tests under thechrono
feature. This allows otherbuiltins
features to be used, while not bringing in thechrono
dependency.#706
https://www.reddit.com/r/rust/comments/qamgyh/is_the_chrono_crate_unmaintained/