config-rs
config-rs copied to clipboard
fix: Change YAML crate to `serde_yaml`
-
yaml-rust
is unmaintained for over 2 years. - Like with
yaml-rust
, requires special handling for table keys (ensuring numbers are converted to strings).
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:
- Prior to the
0.9
release (July 2022) ofserde_yaml
, it used theyaml-rust
crate internally. Now it uses a transpiledlibyaml
crate. - The
yaml-rust
format had parsing logic byconfig-rs
to ensure a single document or create an empty Map. You should get similar withserde_yaml
for an empty Map orValueKind::Nil
-
Numbers can be keys, but have always been converted to strings for the
Map
type inconfig-rs
.- There is an existing test-case YAML config for this with mixed number and string keys. No difference in behaviour here than what we already had.
- There is a tracking issue at
serde_yaml
, ideally that would be resolved upstream to properly handle number to string conversion (only seems to be an issue for untagged enums).
-
serde_yaml
changed tagged enums with0.9
release, that is causing some issues from users upgrading from0.8
.config-rs
test suite is passing, but the test coverage for config files input to Rust types may not cover some of these concerns? 🤷♂️ - Some YAML parsers have a wider range of values that are treated as booleans based on YAML 1.1 spec apparently, while
serde_yaml
only implements support for what the YAML 1.2 spec states should be treated as bools.- A request for wider support was rejected.
- It may be encountered in some configs like Ansible, where it's considered a valid bool value type. I've not checked if
yaml-rust
is any better at this though.
- What may look like an invalid config, may be treated as a single
String
instead of throwing an error depending on the input, as it's technically valid.
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 🤷♂️
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!
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.
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.
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.
I'll self-review again and merge after addressing any existing concerns I raised.
Please do that! Thank you so much for your contributions!