Don't eagerly merge configs. #20459
Merged
Don't eagerly merge configs. #20459
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Merging the config values is incorrect for list- and dict-valued options, where we want to concatenate edits, not stomp them. For example, if for some list-valued option the default value is [0,1], config1 has -[0] and config2 has +[2,3] then the resulting value should be [1,2,3], but before this change it would have been [0,1,2,3].
benjyw
commented
Jan 24, 2024
| config: Value, | ||
| } | ||
|
|
||
| impl ConfigSource { |
There was a problem hiding this comment.
The diff looks scarier than it is. The meat of this change is the introduction of a ConfigSource, representing a single Config, and moving the generic get_value/get_list/get_dict logic to it unchanged. Then Config has a vec of these ConfigSources, and does the right thing.
The rest is just test and plumbing changes.
benjyw
commented
Jan 24, 2024
| @@ -56,24 +56,46 @@ fn test_display() { | |||
| } | |||
|
|
|||
| #[test] | |||
| fn test_section_overlap() { | |||
| // Two files with the same section should result in merged content for that section. | |||
| fn test_multiple_sources() { | |||
There was a problem hiding this comment.
This test has been enhanced to test more multiple source scenarios.
tgolsson
requested changes
Jan 25, 2024
tgolsson
approved these changes
Jan 25, 2024
|
Thanks for the review! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Merging the config values is incorrect for list- and
dict-valued options, where we want to concatenate
edits, not stomp them.
For example, if for some list-valued option the default
value is [0,1], config1 has -[0] and config2 has +[2,3]
then the resulting value should be [1,2,3], but before
this change it would have been [0,1,2,3].
Instead, we preserve each config file as a separate
source, and do the appropriate stomping/merging
at option value get time.