search

tweag / nickel

Better configuration for less
https://nickel-lang.org/
MIT License
2.23k stars 85 forks source link

Add span information for TOML imports #1949

Closed yannham closed 3 weeks ago

yannham commented 3 weeks ago

This PR uses a feature of the toml crate allowing to embed span information into the deserialized data (imported from Nickel). This makes it possible to give nice error messages and point to within a TOML file when the application of a Nickel contract to an existing TOML configuration fails, or in case of non-mergeable values.

Doing so requires a bit of boilerplate and brings an additional crate in (although small). It can also impact deserialization performance, because we need to first deserialize in an intermediate TOML-like datastructure. For those reasons, spanned deserialization has been gated by the spanned-deser feature, which is enabled by default, but can be disabled if this isn't useful.

ErinvanderVeen commented 3 weeks ago

I do wonder whether it's worth the extra feature -- I'd be surprised if there's a measurable impact on performance or binary size. I guess for yaml/json there might be a bigger difference...

I do think it's worth the extra feature. There is a tendency to blow up the dependencies of any given crate. And since the code for when this feature is disabled is so small, why not hide it behind a feature flag. I approve.

I would like to see some "real" performance measurements though.

yannham commented 3 weeks ago

Actual measurement is the path forward, yes. In the meantime, another parameter is that the #[cfg..] attributes are really minimal and easy to insert in this case, so it doesn't cost too much to keep it a feature for now, I think.