mehcode / config-rs

⚙️ Layered configuration system for Rust applications (with strong support for 12-factor applications).
Apache License 2.0
2.43k stars 206 forks source link

fix: Change YAML crate to `serde_yaml` #474

Open polarathene opened 8 months ago

polarathene commented 8 months ago

Resolves: https://github.com/mehcode/config-rs/issues/473


Just some insights below that might be worth noting considering the switch. Presumably this should be delayed until a breaking release is acceptable?

Since config-rs is only interested in deserialization, some concerns users may have regarding serializing valid YAML files may be a non-issue, but changing from one YAML crate to another may behave differently with deserialization still:

polarathene commented 8 months ago

Note, I have implemented the equivalent for https://github.com/mehcode/config-rs/pull/472 which could include the serde_yaml support switch instead if preferred (or addressed as a separate follow-up PR).

Presently the Tagged variant of serde_yaml::Value is not handled. I don't think we can easily implement support for that conversion either? I'm not experienced enough at least to know how to integrate that.

The feature has !Variant as a value prefix in a .yaml file, and serde_yaml represents the type with two properties, tag (that !Variant value) and value actual data for the enum variant.

I'm inclined to ignore it since it's not a previously supported feature 🤷‍♂️

matthiasbeyer commented 8 months ago

First of all, sorry for the delay. I got sick and am writing this from my bed.

So what I see here is a lot of change in behavior when switching from one yaml implementation to another. I am not sure whether I like that, but on the other hand we are in a zero-dot version (0.x.y), so it wouldn't be an issue from a version perspective.

I'll leave it up to you, really!

polarathene commented 8 months ago

I got sick and am writing this from my bed.

Ah, no worries rest up 👍

I pinged due to your activity appearing active and wasn't sure if you were notified of the PR.


So what I see here is a lot of change in behavior when switching from one yaml implementation to another.

It's roughly at parity, at least the implementation is very similar on config-rs end and what is supported.

I've otherwise made note above of known issues. Some of these may not be relevant to config-rs as it deserializes from YAML input to Value as an intermediate / normalized storage before deserializing to a struct provided by the user? This adds another layer of deserialization where types may be converted to the appropriate type to satisfy the users config struct fields.

on the other hand we are in a zero-dot version (0.x.y), so it wouldn't be an issue from a version perspective.

Tests were helpful and ensured that the switch at least passes those, if there is something that wasn't covered, new tests could be added. It may warrant bumping to a 0.14 release? 🤷‍♂️ (or a part of that bump)

I'll leave it up to you, really!

My preference is to #472 which already bundles the equivalent feature change, but without the serde-yaml specific enum handling.

The yaml format file becomes very simple with one special case for the yaml maps (due to string keys for table being necessary).

There may be some differences between this PR and #472 untagged enum approach, but current tests pass all the same. In the event there is an issue, this PR provides an alternative to fallback to.

matthiasbeyer commented 7 months ago

Note to self:

My preference is to https://github.com/mehcode/config-rs/pull/472 which already bundles the equivalent feature change, but without the serde-yaml specific enum handling.

polarathene commented 7 months ago

Now that the MSRV lock file issue is out of the way, I'll be revisiting my PRs this weekend. If you want to block on any let me know, otherwise I'll self-review again and merge after addressing any existing concerns I raised.

matthiasbeyer commented 7 months ago

I'll self-review again and merge after addressing any existing concerns I raised.

Please do that! Thank you so much for your contributions!