No longer rely on static state in ConfigurationReader #151
Conversation
To pass around the `IConfiguration` object instead of a `static` member
and simplify some method signatures
to avoid checks for null dependencyContext in ConfigurationReader and split responsabilities
and reduce number of arguments to pass around
|
Looks awesome; not sure how the static state there slipped past me, I am not in this headspace enough right now to trust myself to review this PR, but LGTM on the surface! :-) .. Hoping someone with a little more time spare can go through it properly... |
There was a problem hiding this comment.
Hi, @tsimbalar . Looks great overall 👍 . Added some comments. One change is required (regarding DependencyContext.Default check) other probably can be addressed in separate PR(s) if you think so.
...erilog.Settings.Configuration/Settings/Configuration/Assemblies/DllScanningAssemblyFinder.cs
Show resolved
Hide resolved
|
|
||
| public ResolutionContext(IConfiguration appConfiguration = null) | ||
| { | ||
| _declaredLevelSwitches = new Dictionary<string, LoggingLevelSwitch>(); |
There was a problem hiding this comment.
It probably better to initialize Dictionary with case insentitive comparer (to align with other places)
There was a problem hiding this comment.
After reviewing the code, I see that we are not using any case insensitive comparer anywhere where we initialize Dictionarys ... I'd rather not do the change (at least not in this PR), in order not to introduce any subtle bugs.
Shall we open a separate issue to keep track of it ?
src/Serilog.Settings.Configuration/Settings/Configuration/Assemblies/AssemblyFinder.cs
Show resolved
Hide resolved
| @@ -265,11 +253,11 @@ string GetSectionName(IConfigurationSection s) | |||
| } | |||
| } | |||
|
|
|||
| IReadOnlyCollection<Assembly> LoadConfigurationAssemblies() | |||
| static IReadOnlyCollection<Assembly> LoadConfigurationAssemblies(IConfigurationSection section, AssemblyFinder assemblyFinder) | |||
| { | |||
| var assemblies = new Dictionary<string, Assembly>(); | |||
|
|
|||
There was a problem hiding this comment.
It probably better to initialize Dictionary with case insentitive comparer (to align with other places)
There was a problem hiding this comment.
same comment as above :
After reviewing the code, I see that we are not using any case insensitive comparer anywhere where we initialize
Dictionarys ... I'd rather not do the change (at least not in this PR), in order not to introduce any subtle bugs.Shall we open a separate issue to keep track of it ?
src/Serilog.Settings.Configuration/Settings/Configuration/Assemblies/AssemblyFinder.cs
Show resolved
Hide resolved
and protect against null
|
@tsimbalar Thanks! Let push this to the dev to try this in the wild 👍 |
We had a commented out test that failed in a non deterministic way, because of
staticstate.This PR removes that static state altogether, and un-skips that test.
As part of this PR I also did quite a bit of refactoring as I was trying to wrap my head about its behavior :). I also tried to move some code out of
ConfigurationReaderwhich is starting to grow big.ConfigurationReaderpublic to make it clearer that we always expect aConfigSectionand optionally a fullIConfiguration.AssemblyFinderabstract class that takes care of locating assemblies fromDependencyContextor from DLL-scanning, and make it clearer that we try to automatically guess which one to use when not explicitly specified.*.AssembliesnamespaceResolutionContextthat keeps track of the values that can be useful when calling configuration methods (Level switches, the actualIConfigurationobject) that is passed around when processing sub-sections of the config file.ConfigurationReaderclassI did get a bit carried away, and the diffs ends up being quite big and possibly hard to review. If you think it is necessary, I can try and split this PR into multiple commits.
fixes #147