Async support #8
Conversation
|
|
||
| namespace DataDog.Tracing.Sql | ||
| { | ||
| public class TraceDbCommand : IDbCommand | ||
| public class TraceDbCommand : DbCommand |
There was a problem hiding this comment.
Using the base classes rather than the interfaces is going to break downstream consumers.
There was a problem hiding this comment.
IDbCommand and IDbConnection are pre .NET 2.0 interfaces, when async/await wasn't still a thing, and therefore lack support for asynchronous methods.
DbCommand and DbConnection were introduced after async/await support, and to avoid breaking previous implementations by adding new functionality Microsoft decided to create those abstract base classes to extend further the interfaces with async methods.
All connectors (MySql connector, SQLServer connector, PostgreSQL Npgsql connector, etc...) implement the abstract classes and not the interfaces nowadays, so this change shouldn't break any consumer (other than those using old .NET versions, but those don't support either .NET Standard so it doesn't even matter because they wouldn't be able to consume this library anyways).
There was a problem hiding this comment.
It certainly breaks the one consumer I know about since they use the interfaces :)
There was a problem hiding this comment.
Do you intend to do the work of fixing this in WB?
There was a problem hiding this comment.
Yes, I'll gladly work in the RepositoryBase implementation of the async overloads and all the Repositories that inherit from it!
It's probably a lot of work but I think it's worth the effort. Also we can leave the synchronous methods for backwards compatibility, maybe with the [ObsoleteAttribute].
There was a problem hiding this comment.
| @@ -1,6 +1,6 @@ | |||
| <Project Sdk="Microsoft.NET.Sdk"> | |||
| <PropertyGroup> | |||
| <TargetFramework>netstandard1.4</TargetFramework> | |||
| <TargetFramework>netstandard2.0</TargetFramework> | |||
There was a problem hiding this comment.
Is this upgrade actually necessary? It's usually better to target as low a netstandard version as you can so that more people can use it.
There was a problem hiding this comment.
Yes, DbCommand and DbConnection classes are not implemented in 1.4.
|
Bumped version to 2.0.0. Is there something else missing, or is it ready to be published into the NuGet package repository? |
Updated all projects to .NET Standard 2.0 / .NET Core 2.2
Added support for async command execution