coreclr: Automatic port of triple slash from System.Buffers.Binary by carlossanlop · Pull Request #2693 · dotnet/dotnet-api-docs (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation104 Commits12 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Choose a reason for hiding this comment

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

Thanks @carlossanlop. Left a few comments to be considered.

Co-Authored-By: Maira Wenzel mairaw@microsoft.com

@mairaw @rpetrusha I resolved all the suggestions, added extra params and returns, clarified the remarks. Can you please take a look?

@layomia since you're the owner, can you also please take a quick look?

Please give me some time to review this before merging. Tyvm.

Please give me some time to review this before merging. Tyvm.

Sure, @ahsonkhan. Please ping me and @rpetrusha when you're done reviewing this PR.

Choose a reason for hiding this comment

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

Made a few more changes. Please take a look.

Your changes look good @mairaw , thanks for making them.
@ahsonkhan will you have a chance to take a look at this PR today? It's one file, and the changes are repetitive, so it should be quick.

@ahsonkhan can we assume this looks good to merge?

mairaw added waiting-on-feedback

Indicates PRs that are waiting for feedback from SMEs before they can be merged

and removed changes-addressed

Indicates PRs that had all comments addressed and are awaiting for new review

labels

Aug 6, 2019

Choose a reason for hiding this comment

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

Other than adding exception tags to document Argument OOR exceptions, the rest of the feedback are nits/questions.

Looks good otherwise.

@mairaw I addressed all of Ahson's comments. He left you a couple of questions (a rewording which I decided to revert), so let me know if you'd like us to use your words instead. Does this look good to merge, if the build finishes successfully?

Choose a reason for hiding this comment

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

Some leftover word-smithing suggestions.

@mairaw the build completed successfully and without warnings. Can we get it merged if the last changes look good to you?

Choose a reason for hiding this comment

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

Use an for Int16/Int32/Int64, but a UInt16/UInt32/UInt64 (consistency with existing a/an usage).

Co-Authored-By: Ahson Khan ahkha@microsoft.com

@mairaw build was successful and with no warnings. The last comments were addressed. Is this good to merge?