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.

Sign up for GitHub

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

Jump to bottom

Add async System.Data resultset and database schema APIs #39098

Merged
merged 1 commit into from
Jul 12, 2020
Merged

Add async System.Data resultset and database schema APIs #39098

merged 1 commit into from
Jul 12, 2020

Conversation

roji
Copy link
Member

@roji roji commented Jul 10, 2020

@Dotnet-GitSync-Bot
Copy link
Collaborator

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

stephentoub
stephentoub approved these changes Jul 11, 2020
View reviewed changes
Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

The ref and src LGTM. Can we add tests?

stephentoub
stephentoub reviewed Jul 11, 2020
View reviewed changes
src/libraries/System.Data.Common/src/System/Data/Common/DbConnection.cs
public virtual DataTable GetSchema(string collectionName, string?[] restrictionValues)
{
throw ADP.NotSupported();
}

/// <summary>
/// This is the asynchronous version of <see cref="GetSchema()" />.
Copy link
Member

Choose a reason for hiding this comment

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

All of this should be in remarks I think. The summary should be similar to the sync version.

Copy link
Member

Choose a reason for hiding this comment

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

(Same for the others)

Copy link
Member Author

Choose a reason for hiding this comment

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

@stephentoub you're right, the docs for all async methods currently point to the sync method and add some notes on the default implementation - not ideal. I've opened #39154 to clean this up globally in System.Data, will take care of that after this PR.

@ghost
Copy link

ghost commented Jul 12, 2020

Tagging subscribers to this area: @roji, @ajcvickers
Notify danmosemsft if you want to be subscribed.

@roji
Copy link
Member Author

roji commented Jul 12, 2020

Added full testing for the new APIs.

@roji roji merged commit 225673d into dotnet:master Jul 12, 2020
@roji roji mentioned this pull request Jul 12, 2020
Closed
@roji roji deleted the GetSchemaAsync branch July 12, 2020 21:20
@jaredpar jaredpar mentioned this pull request Jul 13, 2020
Closed
@karelz karelz added this to the 5.0.0 milestone Aug 18, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stephentoub stephentoub stephentoub approved these changes

Assignees
No one assigned
Labels
area-System.Data new-api-needs-documentation
Projects
None yet
Milestone
5.0.0
Development

Successfully merging this pull request may close these issues.

Async System.Data resultset and database schema APIs
5 participants
@roji @Dotnet-GitSync-Bot @stephentoub @karelz @danmoseley