Update GRM to make use of Dependency Injection
Closed this issue · 11 comments
Detailed Description
Instead of hard coding and passing in objects manually, we should look into updating GRM to make use of Dependency Injection.
Context
Possibly to make maintaining the application easier.
Possible Implementation
Make use of Microsoft.Extensions.DependencyInjection.
Your Environment
Not important
Been looking a little bit about what is necessary to get DI implemented for the repository (using Microsoft.Extension.DependencyInjection).
From what I could gather this seems to require almost a complete rewrite GRM completely, as such I believe this may too big of a task to handle for now (at least for the next release).
I agree, using Microsoft.Extensions.DependencyInjection sounds like the most sensible approach, especially if that is the DI framework used by GitVersion (if push comes to show and there is a need for a different framework, that could extend upon that DI as most provide some kind of wrapper for it nowadays).
Just one thing I believe is worth noting, adding full DI may be difficult to do until issue #126, and issue #190 is implemented. I would be happy to be proved wrong. 😄
I agree, using
Microsoft.Extensions.DependencyInjectionsounds like the most sensible approach, especially if that is the DI framework used by GitVersion (if push comes to show and there is a need for a different framework, that could extend upon that DI as most provide some kind of wrapper for it nowadays).
I made some tests with Microsoft.Extensions.DependencyInjection and had problems with Microsoft.Extensions.Logging. The log messages were displayed async after the ServiceProvider was disposed. I haven't figured out yet how to fix it. And I haven't tested it with Serilog (which I think we should stick with) so I can not say if it behaves differently.
Just one thing I believe is worth noting, adding full DI may be difficult to do until issue #126, and issue #190 is implemented. I would be happy to be proved wrong. 😄
I see it differently. I think it would be a good preparation for this issues. For #126 you mentioned that Spectre.Cli is missing a feature, so it would block the DI implementation.
I see it differently. I think it would be a good preparation for this issues. For #126 you mentioned that Spectre.Cli is missing a feature, so it would block the DI implementation.
Which is also why I mentioned issue #190, we need either the feature implemented, or that issue resolved (removing support for username+password).
Now, to be fair, it is still possible to implement DI support without changing to Spectre.CLI, but since the commandlineparser framework doesn't natively support DI, it may be a difficult process to get it set up in that case.
In terms of "which" DI system to go for, I would suggest that we once again remain consistent with what GitVersion is doing/using. I believe I am correct in saying that they have opted to use
Microsoft.Extensions.DependencyInjection, but perhaps @arturcic or @asbjornu could confirm.
Yes, we use in version 5.x Microsoft.Extensions.DependencyInjection and will be using it in 6.x
@gep13 Issue can be closed.
Agreed, thanks for following up on this one.
🎉 This issue has been resolved in version 0.12.0 🎉
The release is available on:
Your GitReleaseManager bot 📦🚀