Skip to content

Conversation

@divega
Copy link
Contributor

@divega divega commented Jul 18, 2019

cc @carlossanlop, @mairaw, @roji, @ajcvickers

Made a few fixes on the DbDataReader API docs:

  • Removed unnecessary, redundant wording
  • Removed SqlClient-specific content
  • Fixed incorrect descriptions

<returns>The value of the specified column.</returns>
<exception cref="T:System.InvalidOperationException">The connection drops or is closed during the data retrieval.

-or-
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Review these boilerplate remarks.

@carlossanlop carlossanlop requested review from mairaw and rpetrusha July 18, 2019 16:50
@carlossanlop
Copy link
Contributor

Thanks for making this change, Diego. I'm adding Maira and Ron for language review.

@carlossanlop carlossanlop added 🏁 Release: .NET Core 3.0 :checkered_flag: Release: .NET Core 3.0 new-content Indicates PRs that contain new articles waiting-on-reviews Indicates PRs that cannot be merged because of the lack of reviews labels Jul 18, 2019
Copy link

@rpetrusha rpetrusha left a comment

Choose a reason for hiding this comment

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

I've left a number of comments and suggestions for you to consider, @divega. Some of he suggested changes also apply to other occurences in the two files.

divega and others added 3 commits July 19, 2019 15:49
Copy link
Contributor Author

@divega divega left a comment

Choose a reason for hiding this comment

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

Thanks @rpetrusha for all the feedback. I have incorporated most of your suggestions.

<param name="ordinal">The zero-based column ordinal.</param>
<summary>Returns the provider-specific field type of the specified column.</summary>
<summary>Gets the provider-specific type of the specified column.</summary>
<returns>The <see cref="T:System.Type" /> object that describes the data type of the specified column.</returns>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I prefer to mention that returns the type (maybe say the .NET type to differentiate from the database type) and leave out the word object. I will make that change, you can let me know later how it looks like for you.

@divega
Copy link
Contributor Author

divega commented Jul 19, 2019

@rpetrusha, @carlossanlop, @mairaw any thought on how useful is having a link to a related article is in here? In the DbDataReader file, the existing link is always the same, an overview of ADO.NET: ~/docs/framework/data/adonet/ado-net-overview.md

@rpetrusha
Copy link

Since it's always the same, @divega, it doesn't seem very useful. I think it's OK to remove it.

Also, did you mean to mark this PR as a draft?

@rpetrusha
Copy link

The build report was showing three broken links, so I've fixed them.

@divega
Copy link
Contributor Author

divega commented Jul 22, 2019

Also, did you mean to mark this PR as a draft?

Yea, it was the intention initially, but now I think it is getting closer to ready.

@divega divega marked this pull request as ready for review July 22, 2019 19:49
@divega divega requested a review from stevestein as a code owner July 22, 2019 19:49
@mairaw mairaw added this to the July 2019 milestone Jul 24, 2019
Copy link
Contributor Author

@divega divega left a comment

Choose a reason for hiding this comment

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

I have done another pass in which I:

  • Made exceptions thrown more consistent
  • Removed async default implementation details from summary
  • Improved returned value description for more async methods
  • Created semi-standard remark for async methods that I expect to use a lot in my next PR, so I would grateful if I can get some feedback. This is the most complete version (in reality not all sentences apply to all methods):

Remarks

This asynchronous method is only needed to avoid blocking the calling thread when the reader is created in sequential mode.

If sequential mode isn't specified, all column values should become available in memory each time ReadAsync completes, and calling the synchronous version of the method shouldn't block the calling thread.

The default implementation of this asynchronous method invokes its synchronous counterpart and returns a completed Task, potentially blocking the calling thread. The default implementation also returns a cancelled task if passed an already cancelled cancellation token.

Data providers that support asynchronous programming should override the default inmplementation using asynchronous I/O operations.

This method accepts a cancellation token that can be used to request the operation to be cancelled early. Implementations may ignore this request.

Other methods and properties of the DbDataReader object should not be invoked while the returned Task is not yet completed.

@rpetrusha
Copy link

This looks good, @divega. Thanks for making the additional changes. I'll merge your PR now.

@rpetrusha rpetrusha merged commit 5df7d99 into dotnet:master Jul 29, 2019
@mairaw mairaw removed the waiting-on-reviews Indicates PRs that cannot be merged because of the lack of reviews label Aug 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🏁 Release: .NET Core 3.0 :checkered_flag: Release: .NET Core 3.0 new-content Indicates PRs that contain new articles

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants