update serde_yaml to 0.9 #13
Conversation
There was a problem hiding this comment.
Thanks a lot for your help, it's much appreciated!
Before merging, I'd like to learn more about the carriage return - the commit message doesn't say more either, and as an outcome of this the explainer or current understanding of it can go into the commit message as well.
I am quite aware that this might just be akin to the k[4..] that was used before, but I'd like to be sure.
Thanks again.
src/value/serde_yaml.rs
Outdated
| v, | ||
| ) | ||
| let k = to_string(k).expect("yaml value to serialize into yaml correctly"); | ||
| let k = &k[..k.len() - 1]; // trim trailing carriage return |
There was a problem hiding this comment.
Can you elaborate why this assumption was added? Particularly, how can we be sure this is truly always the case.
There was a problem hiding this comment.
I had to modify the k[4..] while updating treediff to serde_yaml 0.9 because the output string produce by to_string() is not including any padding anymore.
Then I found, as mentioned in 6d524b4, that the \n in the key value is not correct for a string key. I've implemented a function in my project that retrieve a Value based on the keys provided by treediff, and I had to remove the trailing \n in my code to make it work. So I thought it might be good to fix it too.
But now that I better understand the code, I think I should have used serde_yaml::from_str() in my code in order to support key types that are not strings. In that case the \n is not much problematic, it's just cosmetic. I'll modify the PR to check if there is a trailing \n before removing it, so we can be sure it will always work.
glehmann
force-pushed
the
serde-yaml-09
branch
from
4ad230f to
f42592e
Compare
There was a problem hiding this comment.
Thanks for looking into this!
I am still not convinced it's the right choice to remove trailing newlines because they are inconvenient though. A key is a key as presented by the underlying decoder.
An argument could be made that previously treediff was fine removing 4 leading spaces which it considered buggy enough to remove, but it never removed trailing newlines and I am just afraid this opens another can of worms and breaks some downstream that, who knows why, relies on these to be present.
If the key adjustment would be removed, I could merge this PR right away.
src/value/serde_yaml.rs
Outdated
| v, | ||
| ) | ||
| let mut k = to_string(k).expect("yaml value to serialize into yaml correctly"); | ||
| if k.ends_with('\n') { |
There was a problem hiding this comment.
Is there a chance that these are ever \r\n on Windows?
I really don't think that this should be fixed here - if serde_yaml returns these, there must be something to it, right?
To me it seems fair that users of SerdeYaml have to deal with this information, and the treediff doesn't degrade it.
There was a problem hiding this comment.
ok, I'm removing this commit 👍
glehmann
force-pushed
the
serde-yaml-09
branch
from
f42592e to
3177d4a
Compare
There was a problem hiding this comment.
Great, thanks a a lot.
The new release is here: https://github.com/Byron/treediff-rs/releases/tag/v4.0.3
|
thanks! |
ref: #11