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

Document difference in DateTime vs TimeSpan value for DbType.Time parameter #1173

Merged
merged 2 commits into from
Jul 23, 2021

Conversation

natehitze
Copy link
Contributor

Added the only difference in functionality I've found so far, from #1164.

Added the only difference in functionality I've found so far, from dotnet#1164.
@JRahnama JRahnama added the Area\Documentation Issue that requires changes in public documentations/samples. label Jul 14, 2021
Added DbType.Date note
@cheenamalhotra
Copy link
Member

Hey @ErikEJ

Would like to request your review too 😊

@ErikEJ
Copy link
Contributor

ErikEJ commented Jul 15, 2021

Should all breaking changes be documented on this page? Maybe better to link to the breaking changes in release notes:

https://github.com/dotnet/SqlClient/blob/main/release-notes/2.0/2.0.0.md#breaking-changes-1

https://github.com/dotnet/SqlClient/blob/main/release-notes/3.0/3.0.0.md#breaking-changes-1

I wonder if this should have been documented as a partly breaking change in 1.1 release notes?

There are hints here: #269 (comment) and here: #434 that maybe it should.

Also the 1.1 release notes do not seem to be consolidated with the 1.1 previews: https://github.com/dotnet/SqlClient/blob/main/release-notes/1.1/1.1.0.md (so the fix for #5 is not even mentioned)

@cheenamalhotra
Copy link
Member

That's unfortunate :(

I agree giving link to breaking changes is definitely a good idea. But also calling out these important changes in comparison to System.Data.SqlClient that matter the most while porting would also help readers catch stuff they should really care about.

We can take care of remaining changes separately, this PR can focus only on Date/Time related changes. I wanted to run by you to ensure details are correct.

@ErikEJ
Copy link
Contributor

ErikEJ commented Jul 16, 2021

I think merging this without linking to the breaking changes sections would be misleading.

Otherwise LGTM

@natehitze
Copy link
Contributor Author

IMO linking to the breaking changes actually could be misleading in the context of this document, unless I'm misunderstanding the point of the breaking changes doc. My assumption is that breaking changes sections documents what breaks between versions of this library which may or may not apply to a conversion from System.Data.SqlClient (e.g. breaking changes in features that were only introduced in Microsoft.Data.SqlClient).

My suggestion would be to comb through the release notes and copy breaking changes that apply from there to here... but I don't know enough about the libraries to accomplish that.

@ErikEJ
Copy link
Contributor

ErikEJ commented Jul 16, 2021

I had a look through, and none of the other changes seem important!

@cheenamalhotra cheenamalhotra merged commit 6a5a670 into dotnet:main Jul 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area\Documentation Issue that requires changes in public documentations/samples.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants