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

Consider adding string overloads of DbDataReader.GetFieldValueAsync and DbDataReader.IsDBNullAsync #28800

Closed
divega opened this issue Feb 27, 2019 · 3 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Data
Milestone

Comments

@divega
Copy link
Contributor

divega commented Feb 27, 2019

When we reviewed the API for #27059 we decided to leave async methods for later, for various reasons:

  • It is less commonly used API (only required for sequential access)
  • We were hoping to come up with some new API pattern for handling nullability that takes advantage of the new support for nullable reference types in C#
  • We had doubts on whether new fine-grained async API should return ValueTask<T> instead of Task<T>

However, given that we haven't made progress towards a new API pattern for nullability, and that this async API isn't that commonly used, it seems to me that adding the corresponding string versions for the two existing async methods is the simplest way to complete the improvement we started in #27059.

Proposed API:

 public abstract class DbDataReader
 {
    public Task<T> GetFieldValueAsync<T>(string name) => GetFieldValueAsync(GetOrdinal(name));
    public Task<bool> IsDBNullAsync(string name) => IsDBNullAsync(GetOrdinal(name));
}

cc @roji @ajcvickers

@divega divega changed the title Add string overloads of DbDataReader.Get Add string overloads of DbDataReader.GetFieldValueAsync and DbDataReader.IsNullAsync Feb 27, 2019
@divega divega changed the title Add string overloads of DbDataReader.GetFieldValueAsync and DbDataReader.IsNullAsync Add string overloads of DbDataReader.GetFieldValueAsync and DbDataReader.IsDBNullAsync Feb 27, 2019
@divega divega changed the title Add string overloads of DbDataReader.GetFieldValueAsync and DbDataReader.IsDBNullAsync Consider adding string overloads of DbDataReader.GetFieldValueAsync and DbDataReader.IsDBNullAsync Feb 27, 2019
@roji
Copy link
Member

roji commented Feb 27, 2019

I think the idea was to give a bit more time to see what happens with dotnet/csharplang#2194 (for nullability). Aside from that it definitely seems wise to have column-access methods return ValueTask rather than Task - although in this specific case it would make the API inconsistent (ordinal overload returns Task, string overload returns ValueTask).

Another way to look at this would be to add these two overloads now, at the very least for API consistently (why are they missing whereas all the others are there). Later, when we have a clear answer on the nullability issue we can add new methods (with new names) which would return ValueTask. This path wouldn't add any additional clutter as the ordinal overloads which return Task are already there...

@divega
Copy link
Contributor Author

divega commented Feb 28, 2019

@roji Also, if we are having ValueTask<T> returning versions of these, then the version that is virtual and takes the ordinal int needs to exist first. If we do something with nullabilily, that that is also a cross-cutting concern that is going to affect all sync and async versions, as well as string and int flavors of the methods.

So yes, I only created this issue for the short term goal of making things a bit more consistent, which seems to be low hanging fruit.

@roji
Copy link
Member

roji commented Mar 5, 2019

Closing in favor of #27059 which includes this now.

@roji roji closed this as completed Mar 5, 2019
@msftgits msftgits transferred this issue from dotnet/corefx Feb 1, 2020
@msftgits msftgits added this to the 3.0 milestone Feb 1, 2020
@dotnet dotnet locked as resolved and limited conversation to collaborators Dec 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Data
Projects
None yet
Development

No branches or pull requests

3 participants