Merge Npgsql.DependencyInjection into Npgsql by roji · Pull Request #6594 · npgsql/npgsql

Copilot

Choose a reason for hiding this comment

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

Do we actually need an explicit reference here?

Choose a reason for hiding this comment

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

We indeed don't, though given that this PR introduces a direct dependency on DI into Npgsql, it kinda makes sense to me to have a direct nuget dependency too, rather than relying on logging to "accidentally" bring it in... Though ultimately I don't think it makes a difference either way.

Any reason you'd rather remove it? Am OK to go either way.

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

I think the explicit reference is better. Although the chances of Microsoft.Extensions.DependencyInjection.Abstractions being removed from the dependencies of Microsoft.Extensions.Logging.Abstractions are really low (it was introduced as a dependency in Microsoft.Extensions.Logging.Abstractions 8.0.0) it can become a problem when consumer choose explicit versions of the Microsoft.Extensions.* dependencies.

I think it was a mistake to make Microsoft.Extensions.Logging.Abstractions depend on Microsoft.Extensions.DependencyInjection.Abstractions but anyway, Microsoft.Extensions.DependencyInjection itself was a mistake 🤷🏻‍♂️.

Choose a reason for hiding this comment

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

Oops, missed this one, sorry.
Overall, not really important whether we reference it explicitly or not, but personally I do not see any particular reason as to why we should do that. If at any point Microsoft.Extensions.Logging.Abstractions will stop referencing Microsoft.Extensions.DependencyInjection.Abstractions, we'll notice this immediately as the project will stop compiling, If it doesn't, we have one less package for dependabot to track.
Anyway, don't mind whatever you choose.