Config.Root should contain fallback values #181
Merged
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
IgorFedchenko
commented
Jan 16, 2020
| } | ||
| catch | ||
| { | ||
| if (throwIfNotFound) | ||
| throw; | ||
|
|
||
| result = Fallback?.GetNode(path); |
There was a problem hiding this comment.
No need to check for fallback here anymore - if Root does not have it, then it just does not exist
There was a problem hiding this comment.
Much better - I like this change. Makes sense.
IgorFedchenko
commented
Jan 16, 2020
| @@ -123,12 +138,6 @@ public virtual Config GetConfig(string path) | |||
| public virtual Config GetConfig(HoconPath path) | |||
| { | |||
| var value = GetNode(path); | |||
| if (Fallback != null) | |||
There was a problem hiding this comment.
Same as above, since GetNode is loading node from Root, which already has all fallback info.
IgorFedchenko
commented
Jan 16, 2020
| @@ -188,19 +197,13 @@ public virtual Config WithFallback(Config fallback) | |||
| public override IEnumerable<KeyValuePair<string, HoconField>> AsEnumerable() | |||
| { | |||
| var used = new HashSet<string>(); | |||
| var current = this; | |||
| while (current != null) | |||
| foreach (var kvp in Root.GetObject()) | |||
There was a problem hiding this comment.
Since now Root includes fallbacks, no need to iterate them
Aaronontheweb
approved these changes
Jan 16, 2020
| } | ||
| catch | ||
| { | ||
| if (throwIfNotFound) | ||
| throw; | ||
|
|
||
| result = Fallback?.GetNode(path); |
There was a problem hiding this comment.
Much better - I like this change. Makes sense.
| } | ||
|
|
||
| [Fact] | ||
| public void HoconValue_GetObject_should_use_fallback_values_with_complex_objects() |
There was a problem hiding this comment.
Glad to see some more complex test cases. This is great work.
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.
Close #177
The issue turned out to be little bit deeper then I thought at start.
Assuming that
confighas fallback values,config.Root.GetObject()returns object not containing fallback values becauseRootproperty of typeHoconValuedoes not contain them.Hocon core library does not work with fallbacks in any way - so
HoconValuedoes not have any information about possible fallbacks it has.So the way to go is to aggregate all fallback values into Root property, which is what I am doing. Since
HoconValuetype is mutable, making separate value and putting values in reverse order - to support proper fallbacks ordering when hocon will merge inner objects.Also, seems like few more functions in
Configfile were trying to check if any fallbacks exist - there is no need to do this anymore, since we always have magicRootvalue that already contains aggregatedHoconValuewith all fallbacks included.Added more tests to verify that deep objects merge correctly. And, important thing - fixed
config.ToString(useFallbackValues: true)implementation, since old one was dropping original values and was only using fallback values. Updated test for this as well.