feat: Add support for float and boolean hash keys #650
Conversation
|
Seems we should turn that panic into an error |
| @@ -50,6 +50,8 @@ fn from_yaml_value( | |||
| match key { | |||
| yaml::Yaml::String(k) => m.insert(k.to_owned(), from_yaml_value(uri, value)?), | |||
| yaml::Yaml::Integer(k) => m.insert(k.to_string(), from_yaml_value(uri, value)?), | |||
| yaml::Yaml::Real(k) => m.insert(k.to_string(), from_yaml_value(uri, value)?), | |||
There was a problem hiding this comment.
My main concern with floats is that this isn't roundtrippable which matters for whether you can predict the key from the file.
There was a problem hiding this comment.
Good point, do you have a suggestion on how to handle that? Is there a "pass through unchanged" option we can convince the underlying yaml parser to do?
Alternatively, is supporting float keys in a way that's not roundtrippable better than not supporting them at all? The use case of assuming the user will parse the resulting string into a float should be close to what they would expect (within some float precision error) even if the string ends up being different. We'd have to document it.
There was a problem hiding this comment.
Alternatively, is supporting float keys in a way that's not roundtrippable better than not supporting them at all?
That depends. If we make a feature that will randomly break on people in unpredictable ways, is it better to not have the feature at all? That is what we need to better understand.
The use case of assuming the user will parse the resulting string into a float should be close to what they would expect (within some float precision error) even if the string ends up being different. We'd have to document it.
My concern is someone puts 0.1 in their yaml file and "0.1" in their code and expect it to work but instead the key is "0.1000000000001".
There was a problem hiding this comment.
btw we could separate out these questions and merge the bool and panic->error parts of this now.
MartinHowarthFlexciton
force-pushed
the
handle-float-and-bool-hash-keys
branch
from
c00edcd to
afba1e0
Compare
|
I've turned the remaining unsupported cases into a useful error message as you suggested |
| @@ -338,3 +338,71 @@ fn yaml() { | |||
| let date: DateTime<Utc> = s.get("yaml_datetime").unwrap(); | |||
| assert_eq!(date, Utc.with_ymd_and_hms(2017, 6, 12, 10, 58, 30).unwrap()); | |||
| } | |||
|
|
|||
| #[test] | |||
| fn test_yaml_parsing_float_hash() { | |||
There was a problem hiding this comment.
Sorry, I forgot to call out that we generally ask for tests to be added in their own commit before behavior changes, passing. This shows the current behavior (e.g. should_panic). Then in the behavior-change commit, the diff of the test shows how behavior changed. This helps make sure we are testing the right thing and is one of the clearest ways to demonstrate the intent of a PR.
There was a problem hiding this comment.
(sorry, forgot to call that out earlier)
| _ => unreachable!(), | ||
| yaml::Yaml::Real(k) => m.insert(k.to_string(), from_yaml_value(uri, value)?), | ||
| yaml::Yaml::Boolean(k) => m.insert(k.to_string(), from_yaml_value(uri, value)?), | ||
| other => Err(Box::new(UnsupportedHashKeyError(format!("{other:?}"))))?, |
There was a problem hiding this comment.
Ideally the error change would be in its own commit
The
unreachable!code at https://github.com/rust-cli/config-rs/blob/main/src/file/format/yaml.rs#L53 is reachable, and this PR fixes it for two simple types: reals and booleans.Prior to this change you'd get this error trying to parse the following:
The problematic case for me turned out to be float keys:
The change in this PR follows the same pattern as already exists for Strings and Integers - just parse them as strings and leave it up to the user to work out what to do with that.
Personally I only need the Real support, so happy to remove the Boolean case if that's at all contentious.
I decided not to look into whether you can craft a hash with the other Yaml types (below) because it's such an odd use case it didn't seam realistic! That said, #547 suggests that just treating Hash as string would have avoided that users' problem. However, converting from these types back into the exact string format they were parsed in would be difficult, so it's not clear what the right implementation would be. So getting the simpler case (reals, booleans) done feels like the right sized change to make.
Let me know what you think! And thanks for maintaining this useful crate :)