Skip to content

Conversation

@carlossanlop
Copy link
Contributor

@JeremyKuhne wrote these APIs. @layomia is the main owner. @ahsonkhan is third owner.

Here's the API proposal: https://github.com/dotnet/corefx/issues/32588
Here's the PR where they were introduced: https://github.com/dotnet/corefx/pull/33288/files

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

The build says there was an error in SequenceReaderExtensions.xml: "Object reference not set to an instance of an object", but there is no error line and doesn't say which object. In the last table, it says the file succeeded validation. I'm not sure what happened. @rpetrusha @mairaw how do I find out what caused the error?

@mairaw mairaw closed this Jul 26, 2019
@mairaw mairaw reopened this Jul 26, 2019
@mairaw
Copy link
Contributor

mairaw commented Jul 26, 2019

Let's try rebuilding first (closing and reopening the PR does that). If the issue doesn't go away, we might have to file a bug. I haven't seen this issue before.

@rpetrusha
Copy link

The build failed with no indication of an underlying cause. Closing and reopening to start new build.

@rpetrusha rpetrusha closed this Jul 26, 2019
@rpetrusha rpetrusha reopened this Jul 26, 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.

This looks good, @carlossanlop. I've made two minor changes to the SequenceReaderExtensions type summary. I'll merge when the build completes successfully.

@rpetrusha rpetrusha added verify-build-before-merge and removed waiting-on-reviews Indicates PRs that cannot be merged because of the lack of reviews labels Jul 29, 2019
Copy link
Contributor

@ahsonkhan ahsonkhan left a comment

Choose a reason for hiding this comment

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

Otherwise, LGTM.

@rpetrusha
Copy link

There are a couple of comments and suggestions from @ahsonkhan to address before we merge, @carlossanlop.

@carlossanlop
Copy link
Contributor Author

All suggestions applied. If this looks good after the build finishes, can we get it merged? @rpetrusha

@carlossanlop
Copy link
Contributor Author

@rpetrusha the build passed. I think it's good to merge.

@rpetrusha
Copy link

I'll merge it now, @carlossanlop.

@rpetrusha rpetrusha merged commit 87c3b84 into dotnet:master Aug 6, 2019
@carlossanlop carlossanlop deleted the System.Buffers.SequenceReader branch August 6, 2019 17:06
<param name="value">To be added.</param>
<summary>To be added.</summary>
<returns>To be added.</returns>
<param name="reader">The byte sequence reader from which to read the value.</param>
Copy link
Contributor

Choose a reason for hiding this comment

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

All instance of this weren't fixed :(

<param name="value">To be added.</param>
<summary>To be added.</summary>
<returns>To be added.</returns>
<param name="reader">The byte sequence reader instance from which the value is to be read.</param>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<param name="reader">The byte sequence reader instance from which the value is to be read.</param>
<param name="reader">The byte sequence reader from which to read the value.</param>

<summary>Reads a big-endian <see cref="T:System.Int16" />.</summary>
<returns>
<see langword="true" /> if the read operation is successful; <see langword="false" /> if there isn't enough data for an <see cref="T:System.Int16" />.</returns>
<param name="reader">The byte sequence reader instance from which the value is to be read.</param>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<param name="reader">The byte sequence reader instance from which the value is to be read.</param>
<param name="reader">The byte sequence reader from which to read the value.</param>

<param name="value">To be added.</param>
<summary>To be added.</summary>
<returns>To be added.</returns>
<param name="reader">The byte sequence reader instance from which the value is to be read.</param>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<param name="reader">The byte sequence reader instance from which the value is to be read.</param>
<param name="reader">The byte sequence reader from which to read the value.</param>

<summary>Reads a big-endian <see cref="T:System.Int32" />.</summary>
<returns>
<see langword="true" /> if the read operation is successful; <see langword="false" /> if there isn't enough data for an <see cref="T:System.Int32" />.</returns>
<param name="reader">The byte sequence reader instance from which the value is to be read.</param>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<param name="reader">The byte sequence reader instance from which the value is to be read.</param>
<param name="reader">The byte sequence reader from which to read the value.</param>

<summary>Reads a little-endian <see cref="T:System.Int16" />.</summary>
<returns>
<see langword="true" /> if the read operation is successful; <see langword="false" /> if there isn't enough data for an <see cref="T:System.Int16" />.</returns>
<param name="reader">The byte sequence reader instance from which the value is to be read.</param>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<param name="reader">The byte sequence reader instance from which the value is to be read.</param>
<param name="reader">The byte sequence reader from which to read the value.</param>

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.

6 participants