Skip to content

Conversation

ajcvickers
Copy link

Fixes #609 by allowing the configuration paths to be associated with values to help with debugging.

@davidfowl
Copy link
Member

What does it look like? Show an example?

@ajcvickers ajcvickers requested a review from HaoK December 16, 2018 04:32
@ajcvickers
Copy link
Author

@davidfowl Here's an example:

Key1=Value1 (JsonConfigurationProvider for 'C:\Stuff\myconfig.json' (Optional))
Section1:
  Key2=Value12 (MemoryConfigurationProvider)
  Section2:
    Key3=Value123 (MemoryConfigurationProvider)
    Key3a:
      0=ArrayValue0 (JsonConfigurationProvider for 'C:\Stuff\myconfig.json' (Optional))
      1=ArrayValue1 (JsonConfigurationProvider for 'C:\Stuff\myconfig.json' (Optional))
      2=ArrayValue2 (JsonConfigurationProvider for 'C:\Stuff\myconfig.json' (Optional))
Section3:
  Section4:
    Key4=Value344 (MemoryConfigurationProvider)

@HaoK
Copy link
Member

HaoK commented Dec 17, 2018

Nice, did you happen to try the KeyPerFile provider? I'm curious what that looks like in debug

@ajcvickers
Copy link
Author

@HaoK Every provider other than XML produces this output for the common test data:

Key1=Value1 ({providerTag})
Section1:
  Key2=Value12 ({providerTag})
  Section2:
    Key3=Value123 ({providerTag})
    Key3a:
      0=ArrayValue0 ({providerTag})
      1=ArrayValue1 ({providerTag})
      2=ArrayValue2 ({providerTag})
Section3:
  Section4:
    Key4=Value344 ({providerTag})

Where providerTag is what you get from calling ToString on the provider. For KeyPerFile, ToString is currently:

=> $"{GetType().Name} ({(Source.Optional ? "Optional" : "Required")})";

But we should probably put the filename in there as well. (It doesn't look easily accessible right now, but that could be changed.)

@HaoK
Copy link
Member

HaoK commented Dec 17, 2018

What do you think about implementing ToString on every IConfigurationSource as well to provide the debug view and just calling Source.ToString() inside of that debug block? Sources are what you add to the config builder and would contain all of the settings

@ajcvickers
Copy link
Author

@HaoK Sounds reasonable. I'll make that change.

@ajcvickers
Copy link
Author

@HaoK Doesn't look like there is a public or even common way to get from a provider to the source.

Fixes #609 by allowing the configuration paths to be associated with values to help with debugging.
@ajcvickers ajcvickers force-pushed the SusurrousOrConflagration1204 branch from 4d47635 to a70bcd3 Compare December 18, 2018 00:07
@ajcvickers
Copy link
Author

@HaoK Pushed an update that includes the directory from which the key files are loaded in the debug output.

@HaoK
Copy link
Member

HaoK commented Dec 18, 2018

@ajcvickers I was just suggesting modifying the ToString in the providers i.e. in FileConfigurationProvider:

// FileConfigurationProvider (move this to the base ConfigurationProvider since it would now be the same?)
public override string ToString()
             => $"{GetType().Name} for '{Source.ToString()}'";

And move the Source printing to that whichever specific ones that add info
// FileConfigurationSource
public override string ToString()
             => $"'{Source.Path}' ({(Source.Optional ? "Optional" : "Required")})";

@ajcvickers
Copy link
Author

@HaoK What's the advantage?

@HaoK
Copy link
Member

HaoK commented Dec 18, 2018

Its not that different I guess since the providers and sources are always a pair, but the settings are more a concern of the source than the provider, that was the idea behind splitting them out anyways.

I was just thinking for the new StreamProvider, if the base ConfigurationProvider implemented ToString() as that, I would only have to implement ToString() on the sources, but its not a big deal either way.

The tradeoff mostly is between whether we want to have most Sources implement ToString(). Which I think we actually should have every source print out something.

MemoryConfigurationSource could print out something small too like { InitialData with X entries } and ChainedConfigurationSource could do something like {IConfiguration with X providers}

@ajcvickers
Copy link
Author

@HaoK Maybe you should take a look while doing the stream work and refactor then if it makes sense to you?

@HaoK
Copy link
Member

HaoK commented Dec 18, 2018

Sounds good, this should be fine to merge I can tweak in my PR (although that might be a while unless you think I should decouple the stream changes from the new json stack?)

@ajcvickers
Copy link
Author

@HaoK Let's not separate them yet...

@ajcvickers ajcvickers merged commit d7f8e25 into master Dec 18, 2018
@natemcmaster natemcmaster deleted the SusurrousOrConflagration1204 branch December 31, 2018 17:35
@ghost ghost locked as resolved and limited conversation to collaborators May 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants