-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Lib updates and prepare for Microsoft.Data.SqlClient #1313
Conversation
Suggest look at dotnet/SqlClient#30 (comment) |
…an via a hard-coded handler (AddSqlDataRecordsTypeHandler)
…eaking change, so rev the semver
… which Microsoft.Data tests *will never work*
</dependentAssembly> | ||
</assemblyBinding> | ||
</runtime> | ||
</configuration> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be able to delete this app.config
and it generate as the <name>.config
file in the output directory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope, doesn't generate the binding for Microsoft.SqlServer.Types if you do that; is needed
@@ -20,6 +20,6 @@ | |||
<Reference Include="Microsoft.CSharp" /> | |||
</ItemGroup> | |||
<ItemGroup Condition="'$(TargetFramework)' == 'netstandard1.3'"> | |||
<PackageReference Include="Microsoft.CSharp" Version="4.3.0" /> | |||
<PackageReference Include="Microsoft.CSharp" Version="4.5.0" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's drop netstandard1.3
from all builds
Dapper.Tests/Dapper.Tests.csproj
Outdated
</ItemGroup> | ||
<ItemGroup Condition="'$(Platform)'=='x86'"> | ||
<Content Include="$(USERPROFILE)\.nuget\packages\microsoft.sqlserver.types\14.0.1016.290\nativeBinaries\x86\*.dll" | ||
Condition="'$(TargetFramework)' == 'net46' OR '$(TargetFramework)' == 'net472'"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move these conditions to the ItemGroup
(same above)
Dapper.Tests/Dapper.Tests.csproj
Outdated
<ItemGroup Condition="'$(TargetFramework)' == 'netcoreapp2.0'"> | ||
<PackageReference Include="Microsoft.Data.Sqlite" Version="2.0.0" /> | ||
<PackageReference Include="Microsoft.Data.Sqlite" Version="2.2.6" /> | ||
</ItemGroup> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can drop this block with netcoreapp2.0
build drop
} | ||
finally | ||
{ | ||
connection.Execute("DROP TYPE int_list_type"); | ||
try { connection.Execute("DROP TYPE int_list_type"); } catch { } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's double check if we need these with the collections defined now
Dapper.Tests/ParameterTests.cs
Outdated
@@ -686,6 +741,8 @@ public void DBGeography_SO24405645_SO24402424() | |||
[Fact] | |||
public void SqlGeography_SO25538154() | |||
{ | |||
if (IsMsDataClient) return; // not supported |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Conflicts: # Dapper/SqlMapper.cs
…overhead, subclass Identity so we don't always need the arrays
…1313) v2 work; primarily prep for split SqlClient
Intent: make Dapper usable with both System.Data.SqlClient and Microsoft.Data.SqlClient
Details:
IEnumerable<System.Data.SqlDataRecord>
toIEnumerable<T> where T : IDataRecord
; this makes it a breaking change - although one that is very unlikely to impact many people at all, so:Note: the removal of System.Data.SqlClient may impact consumers who depended on Dapper to get that ref, but I strongly consider removing it a GoodThing™ for all; folks can add the correct package now. Will need a release note.
Re the semver bump; my view here is: