treat nil parents as empty tables if required #627
Conversation
sunshowers
force-pushed
the
empty-default
branch
2 times, most recently
from
3c16f8f to
39f86b4
Compare
Pull Request Test Coverage Report for Build 12776160763Details
💛 - Coveralls |
|
Great, I can confirm that this fixes the regression in config-rs 0.15.x. Thank you very much! |
|
@epage apologies about pinging directly -- any chance you can look at this? this is a pretty rough regression, I'm currently planning to pin nextest to an earlier version of config-rs to avoid it |
|
I also had to reject the depency update for one of my projects due to this regression: https://github.com/Holzhaus/helicon/actions/runs/12540222602/job/34967265515?pr=49 |
|
Please keep in mind we just had holidays in the US and I'm still slowly catching up while handling other responsibilities. As for this, two quick thoughts
In this model, the diff of the test between the commits shows how behavior changed, showing the test verifies the right behavior and clearly communicates to reviewers and any one else how the behavior changed |
|
Thanks -- addressing them shortly. |
sunshowers
force-pushed
the
empty-default
branch
2 times, most recently
from
2f095ce to
11f3534
Compare
|
All right, let me know if there are any other changes. Thanks! (Totally understand the delay btw! Just wanted to ensure the severity was understood) |
sunshowers
force-pushed
the
empty-default
branch
2 times, most recently
from
54b3429 to
9f124a3
Compare
In case of an empty table/map, we currently do not resolve to the `Default` impl as we should (specifically for `profile.baz` in the test). A bisect appears to indicate that ec36bff is responsible. The next commit fixes this.
Encountered this regression via nextest's test suite (nextest-rs/nextest#2001). If a table is empty, it would be treated as a unit value. If attempted to be deserialized via a `Default` impl, it would lead to deserialization failing with an error like ```console profile.default-miri: invalid type: unit value, expected struct CustomProfileImpl ``` A bisect appears to indicate that ec36bff is responsible. For empty tables where the Default impl is supposed to work, it no longer would. I've attempted to restore the old behavior of putting in an empty table, specifically this section: rust-cli@ec36bff#diff-c5423e2d2d6c87501239c0304c0f496742e00440defdd20368cf548ba42ab184L175-L178 I'm happy to make changes if there's a better approach.
While the last commit restored behavior to mostly where it was in v0.15.2, this commit introduces a behavior change with the goal of treating empty tables the same as non-empty ones (in particular, `int_to_empty` is now treated the same as `int_to_non_empty`, both of them overwriting the integer with the table). Treating them the same makes a lot of sense logically, and the fact that we weren't doing so is probably a bug in the old implementation. But it is a notable behavior change, and one worth considering carefully.
|
I tested the updated changes and can confirm that config 0.15.6 fixes the issue. Thanks a lot! |
`0.15.5` had a regression that was fixed in `0.15.6` (in [this PR](rust-cli/config-rs#627)), so we need at least that version Closes #534
|
Thanks @sunshowers and @epage for getting this fixed! |
Encountered this regression via nextest's test suite (nextest-rs/nextest#2001). If a table is empty, it would be treated as a unit value. If attempted to be deserialized via a
Defaultimpl, this would lead to deserialization failing with an error likeprofile.default-miri: invalid type: unit value, expected struct CustomProfileImplA bisect appears to indicate that ec36bff is responsible.
I've attempted to restore the old behavior of putting in an empty table, specifically this section:
ec36bff#diff-c5423e2d2d6c87501239c0304c0f496742e00440defdd20368cf548ba42ab184L175-L178
I'm happy to make changes if there's a better approach.
I've also added some tests for a few situations around empty tables. In this case only the last one (
profile.baz) regressed, but I've also added tests for a couple other cases.Fixes #624