Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update GRM to make use of Dependency Injection #181

Closed
AdmiringWorm opened this issue Dec 20, 2019 · 11 comments
Closed

Update GRM to make use of Dependency Injection #181

AdmiringWorm opened this issue Dec 20, 2019 · 11 comments

Comments

@AdmiringWorm
Copy link
Member

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

@AdmiringWorm
Copy link
Member Author

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).

@gep13 gep13 modified the milestones: 0.10.0, 0.11.0 Jan 6, 2020
@gep13 gep13 modified the milestones: 0.11.0, 0.12.0 Mar 16, 2020
@akordowski
Copy link
Contributor

akordowski commented Jul 27, 2020

I would be interested in working this issue. It would also make sense before implementing #254 and #255.

@gep13
Copy link
Member

gep13 commented Jul 30, 2020

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.

@AdmiringWorm
Copy link
Member Author

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. 😄

@akordowski
Copy link
Contributor

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).

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.

@AdmiringWorm
Copy link
Member Author

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.

@arturcic
Copy link
Member

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
Copy link
Member

gep13 commented Jul 31, 2020

@arturcic thanks for confirming.

gep13 added a commit that referenced this issue Aug 24, 2020
@akordowski
Copy link
Contributor

@gep13 Issue can be closed.

@gep13
Copy link
Member

gep13 commented Aug 25, 2020

Agreed, thanks for following up on this one.

@gittools-bot
Copy link

🎉 This issue has been resolved in version 0.12.0 🎉

The release is available on:

Your GitReleaseManager bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants