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

Feature/di2 #1865

Merged
merged 35 commits into from Oct 25, 2019
Merged

Feature/di2 #1865

merged 35 commits into from Oct 25, 2019

Conversation

arturcic
Copy link
Member

@arturcic arturcic commented Oct 18, 2019

That's the continuation of the #1861

What was done:

  • Added Microsoft.Extensions.Hosting package that contains Hosting and DI, Configuration and Logging infrastructure. For now only the Hosting and DI is used.
  • Reworked the Program.cs, now it uses the Generic Host with all the infrastructure goodies.
  • Use the DI (maybe hosting) in the GitVersionTask
  • null checks for injectable instances in constructors.

Copy link
Member

@gep13 gep13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one main question...

src/GitVersionExe/GitVersionApp.cs Outdated Show resolved Hide resolved
src/GitVersionExe/GitVersionApp.cs Outdated Show resolved Hide resolved
Copy link
Member

@gep13 gep13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@arturcic arturcic force-pushed the feature/di2 branch 5 times, most recently from be879e0 to b13e926 Compare October 21, 2019 19:05
src/GitVersionCore/Common/BuildServerResolver.cs Outdated Show resolved Hide resolved
src/GitVersionCore/CoreModule.cs Outdated Show resolved Hide resolved
src/GitVersionCore/ExecuteCore.cs Outdated Show resolved Hide resolved
src/GitVersionCore/ExecuteCore.cs Outdated Show resolved Hide resolved
src/GitVersionCore/GitPreparer.cs Show resolved Hide resolved
src/GitVersionCore/IExecuteCore.cs Outdated Show resolved Hide resolved
src/GitVersionExe/ExecCommand.cs Outdated Show resolved Hide resolved
src/GitVersionExe/ExecCommand.cs Show resolved Hide resolved
src/GitVersionExe/GitVersionRunner.cs Outdated Show resolved Hide resolved
src/GitVersionExe/Module.cs Outdated Show resolved Hide resolved
@asbjornu
Copy link
Member

I love the work you're doing here, @arturcic. Fantastic stuff! 👏 🎈 🎉 ❤️

@arturcic
Copy link
Member Author

@asbjornu, I have fixed the code as you suggested, will continue the work on adding DI.

src/GitVersionCore/Common/BuildServerResolver.cs Outdated Show resolved Hide resolved
src/GitVersionCore/Common/IBuildServerResolver.cs Outdated Show resolved Hide resolved
src/GitVersionCore/Common/IModule.cs Outdated Show resolved Hide resolved
src/GitVersionCore/GitPreparer.cs Outdated Show resolved Hide resolved
src/GitVersionCore.Tests/GitToolsTestingExtensions.cs Outdated Show resolved Hide resolved
src/GitVersionCore/GitVersionComputer.cs Outdated Show resolved Hide resolved
src/GitVersionCore/IGitVersionComputer.cs Outdated Show resolved Hide resolved
src/GitVersionExe/GitVersionExecutor.cs Outdated Show resolved Hide resolved
src/GitVersionExe/IExecCommand.cs Outdated Show resolved Hide resolved
@arturcic
Copy link
Member Author

@asbjornu, I have fixed the code as you suggested

@arturcic
Copy link
Member Author

@asbjornu, I have finished with the DI changes to the code, can you have a last review? Thanks in advance, I know there is a lot of code ;)

@arturcic
Copy link
Member Author

I have addressed all the tasks in the PR description

Copy link
Member

@asbjornu asbjornu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic work, @arturcic! 🙏 🎉 ❤️

src/GitVersionCore/Authentication.cs Show resolved Hide resolved
src/GitVersionCore/Arguments.cs Show resolved Hide resolved
src/GitVersionCore/BuildServers/GitLabCi.cs Outdated Show resolved Hide resolved
src/GitVersionCore/Configuration/ConfigurationUtils.cs Outdated Show resolved Hide resolved
src/GitVersionCore/Configuration/ConfigurationProvider.cs Outdated Show resolved Hide resolved
src/GitVersionExe/IGitVersionExecutor.cs Show resolved Hide resolved
src/GitVersionTask/FileHelper.cs Show resolved Hide resolved
src/GitVersionTask/IGitVersionTaskExecutor.cs Show resolved Hide resolved
src/GitVersionTask/IGitVersionTaskExecutor.cs Outdated Show resolved Hide resolved
@arturcic
Copy link
Member Author

@asbjornu, I have applied the suggestions, so I guess the PR is more or less finished. I would prefer to address the remaining issues as part of other PRs, as I want to keep this PR for the introduction of DI and less for code refactorings. If you don't mind we can merge the code. What do you think?

@arturcic arturcic force-pushed the feature/di2 branch 3 times, most recently from aa55262 to 112e0ca Compare October 25, 2019 19:01
@arturcic
Copy link
Member Author

@asbjornu I will merge the PR. I haave set it to be version 5.1.0-beta

@arturcic arturcic merged commit 2dd6bd1 into GitTools:master Oct 25, 2019
@arturcic arturcic added this to the 5.1.0 milestone Oct 25, 2019
@arturcic arturcic deleted the feature/di2 branch October 28, 2019 07:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants