search

rust-cli / confy

🛋 Zero-boilerplate configuration management in Rust
Other
896 stars 59 forks source link

Add optional compilation option for using YAML instead of TOML #17

Closed Alch-Emi closed 4 years ago

Alch-Emi commented 5 years ago

As I'm sure you know, YAML is a human readable data format very popular for making config files. So popular, in fact, that people have already forked this repository to completely replace TOML with YAML. While replacing TOML with YAML completely on the master branch would probably be a very bad idea, adding it as a compilation option while leaving TOML as the default seems to be a win-win, and will not break compatibility with dependants' config files, (being opt in).

This implementation creates a couple new errors for YAML types with a different name than TOML errors, since renaming TOML errors to a generic name would be a breaking change. However, perhaps next major version bump, BadTomlData and BadYamlData could be merged into DeserializeError or something similar, while SerializeTomlError and SerializeYamlError could be merged into something like SerializeError. This would make someone who already uses TOML's transition to YAML easier, or vice versa.

This implementation also doesn't tolerate having both TOML and YAML, or neither TOML and YAML enabled at once.

Alch-Emi commented 4 years ago

If I might bump this a little, I'd really love some feedback on this. I understand if it's out of scope, or otherwise not something you'all'd like to do, but if it is something that the project would want, I'd love to hear what I can do to get this code up to snuff. I'd be happy to make any changes that y'all see fit!

Dylan-DPC-zz commented 4 years ago

@Alch-Emi Sorry about the delays. If you can rebase, i can merge this

matthiasbeyer commented 4 years ago

This sounds like a nice addition. Maybe we can, in the next step, replace all parsing with serde directly, so that the input type (yaml, toml... whatever) does not even matter anymore?

Would be a huge effort, though...

TheLostLambda commented 4 years ago

I agree with the first of the messages in this thread and think that, if this is merged, it should use SerializeError and DeserializeError instead of separate ones for TOML and YAML.

That could be lumped into the next release with the error reporting overhaul.

Alch-Emi commented 4 years ago

Alright! This should be merge compatible with master. Anything that should go in before it's merged?

TheLostLambda commented 4 years ago

I'd personally like to unify the Serialise and Deserialise errors and bump to 0.4, but I understand if now isn't the time for breaking changes.

Dylan-DPC-zz commented 4 years ago

This won't work this way, since there is a possibility that both features are enabled which will cause chaos (i know users shouldn't be doing it but it doesn't stop them). So we also need to check if the other feature is disabled.

Alch-Emi commented 4 years ago

Oh good catch! I've added a compile error that goes off if someone tries to compile with YAML support without first disabling toml. I've tried to make the error explain pretty well what the user needs to do, and I've made sure that it's the first error to pop up, which is hopefully enough to communicate to people that they need to switch off toml. Do you think that's enough?