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

Depedency Clean-up in csproj #138

Merged
merged 1 commit into from
Aug 8, 2020
Merged

Depedency Clean-up in csproj #138

merged 1 commit into from
Aug 8, 2020

Conversation

thomasgalliker
Copy link
Contributor

@thomasgalliker thomasgalliker commented Aug 6, 2020

I saw that there are some depedency import which can be simplified. The idea is to have the absolute minimum of external nugets referenced here (e.g. use Microsoft.Extensions.Loggging.Abstractions instead of Microsoft.Extensions.Logging as we only use abstractions in this library).

  • Use Abstraction nuget where available (Http+Logging)
  • Use Microsoft.Extensions.Options instead of indirect dependency from Microsoft.Extensions.Logging

* Use Microsoft.Extensions.Options instead of indirect dependency from Microsoft.Extensions.Logging
@cristipufu cristipufu merged commit f4da869 into stefanprodan:master Aug 8, 2020
@thomasgalliker thomasgalliker deleted the csproj-dep-cleanup branch August 10, 2020 11:46
<PackageReference Include="Microsoft.AspNetCore.Http.Abstractions" Version="2.2.0" />
<PackageReference Include="Microsoft.Extensions.Caching.Abstractions" Version="2.2.0" />
<PackageReference Include="Microsoft.Extensions.Logging.Abstractions" Version="2.2.0" />
<PackageReference Include="Microsoft.Extensions.Options" Version="3.1.6" />
Copy link

Choose a reason for hiding this comment

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

Shouldn't the version be 2.2.0 (same as other M.E.* packages)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yes. I gonna have to check that, but you could be right.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fixed it by targeting multiple frameworks: 0897e10

You couldn't have used it in asp.net core 2 applications anymore i assume.

@altso out of curiosity, what were your pain points with that dependency?

Copy link

Choose a reason for hiding this comment

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

I didn't have any issues. I was planning on using this library in my project and was going through the source code to understand the scope/features/dependencies and noticed this inconsistency.

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

Successfully merging this pull request may close these issues.

None yet

3 participants