search

clarkmcc / cel-rust

Common Expression Language interpreter written in Rust
https://crates.io/crates/cel-interpreter
MIT License
377 stars 21 forks source link

Defaulting to features for a compliant CEL interpreter? #108

Closed alexsnaps closed 3 days ago

alexsnaps commented 3 days ago

With the fix of #90, some "standard" features of CEL are now disabled by default. While I agree with the reason and think that providing this fine grained control over dependencies et al is real great to have in any crate, I wonder if such functionality now behind a feature flag, should be default and be an opt-out rather an opt-in. So that anyone grabbing the crate, gets a "std compatible experience" wrt CEL... more so here, as this would be a "slow killer", i.e. only becoming visible at runtime when actually executing some otherwise perfectly fine Expression. wdyt? cc/ @Caellian

i.e. ```diff diff --git a/interpreter/Cargo.toml b/interpreter/Cargo.toml index fa70cce..c9fb30e 100644 --- a/interpreter/Cargo.toml +++ b/interpreter/Cargo.toml @@ -28,6 +28,7 @@ name = "runtime" harness = false [features] +default = ["regex", "chrono"] json = ["dep:base64", "dep:serde_json"] regex = ["dep:regex"] chrono = ["dep:chrono"] ```
alexsnaps commented 3 days ago

Also, unsure about json, unlike regex & chrono, I think this would become visible during development using the crate... so less opinionated whether it should be opt-in or -out... Tho technically JSON (& protobuf) interop are meant to be standard based on my reading of the spec...

Caellian commented 3 days ago

I agree with default = ["regex", "chrono"]. They can be turned off with default-features = false but it makes more sense to have them be enabled by default.

I wouldn't enable json by default only because spec uses "can" and not "must". Using serde as an example: ~70% of crates enable the derive feature but it's not enabled by default because it negatively affects build time. People going the protobuf route might not end up using json anyway...