Remove static logging and replacing with DI-friendly system #483
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
…DI approach for #455 Attempted as much as possible to break few contracts, and provide forwarding to new implementation Removed dependence on Microsoft.Extensions.Logging, but added Microsoft.Extensions.DependencyInjection Works by introducing an internal DI ServiceCollection and ServiceProvider available via a singleton resource. This collection can be configured by a consumer, but services will work out the box without any config (just with Null Logging) Consumers can add own logging, either with AddLogging(), or by telliing xbim to UseLoggerFactory(logFactory). Removed public Setter ILoggers. Because we're not fully in control of DI composition favoured approach is to inject/pass in ILoggerFactory to key roots. We're using ServiceLocator anti-pattern in other cases as a last resort. ModelProvider & Factory has been re-worked to actually work with DI. One subtle change - Previosuly Heuristic ModelProvider kicked in if Esent was loaded. Now that needs to be set explicitly when configuring the app. i.e. XbimServices.Current.ConfigureServices(s => s.AddXbimToolkit(opt => opt.UseHeuristicModel())); Legacy 'DI lite' concepts like IfcStore.ModelProviderFactory.UseHeuristicModelProvider() have been remapped to a new approach and Obsoleted. TODO: many tests use the old obsoleted overloads, which forward onto new . All tests pass (with one flaky IfcStore ModelProvider test to resolve)
andyward
commented
Feb 1, 2023
Caused by random sequencing of tests and shared state
1 task
andyward
commented
Feb 1, 2023
Fixed up defaults on obsolete methods so typical upgraders will auto pick up new implementations by default. i.e. will default to ILoggerFactory signature over ILogger when caller didn't provide any logger Addresses all test warnings except Scanner
…oryModel. Added some guards/logging to Esent & Heurisrstic ModelProvider infrastructure when used under non WIndows o/s
Replaced by ServiceLocator None-breaking as these were only added lately after obsoleting the equivalent ILogger param methods and related DI work
We're mix & match xUnit and vsTest... xUnit classes need Bootstrapping reliably or sequencing issues can trigger failures in singleton state
Made redundant by DI
andyward
changed the title
andyward
changed the title
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.
…DI approach for #455
Attempted as much as possible to break few contracts, and provide forwarding to new implementation
Removed dependence on Microsoft.Extensions.Logging, but added Microsoft.Extensions.DependencyInjection
Works by introducing an internal DI ServiceCollection and ServiceProvider available via a singleton resource. This collection can be configured by a consumer, but services will work out the box without any config (just with Null Logging)
Consumers can add own logging, either with
AddLogging(), or by telling xbim toUseLoggerFactory(logFactory).Removed public Setter ILoggers. Because we're not fully in control of DI composition favoured approach is to inject/pass in ILoggerFactory to key roots. We're using ServiceLocator anti-pattern in other cases as a last resort.
ModelProvider & Factory has been re-worked to actually work with DI.
One subtle change - Previosuly Heuristic ModelProvider kicked in if Esent was loaded. Now that needs to be set explicitly when configuring the app.
i.e.
XbimServices.Current.ConfigureServices(s => s.AddXbimToolkit(opt => opt.UseHeuristicModel()));Legacy 'DI lite' concepts like IfcStore.ModelProviderFactory.UseHeuristicModelProvider() have been remapped to a new approach and Obsoleted.
TODO: many tests use the old obsoleted overloads, which forward onto new . All tests pass (with one flaky IfcStore ModelProvider test to resolve)