Added documentation for APIs in System.Net namespace introduced in 2.x #3280

jozkee · 2019-10-02T00:16:14Z

Documented APIs:

FtpWebResponse
- ContentType
HttpVersion
- Unknown
- Version20
IPAddress (These are all overloads or new APIs introduced for Span)
- ctor(System.ReadOnlySpan{System.Byte})
- ctor(System.ReadOnlySpan{System.Byte},System.Int64)
- Parse(System.ReadOnlySpan{System.Char})
- TryFormat(System.Span{System.Char},System.Int32@)
- TryParse(System.ReadOnlySpan{System.Char},System.Net.IPAddress@)
- TryWriteBytes(System.Span{System.Byte},System.Int32@)

jozkee · 2019-10-02T00:17:11Z

@carlossanlop @mairaw @rpetrusha
@dotnet/ncl

xml/System.Net/IPAddress.xml

mairaw

Thank you @jozkee. Great changes. I left a few suggestions for you.

xml/System.Net/FtpWebResponse.xml

xml/System.Net/IPAddress.xml

scalablecory

behavior is correct; feedback is all about phrasing.

xml/System.Net/IPAddress.xml

scalablecory · 2019-10-04T18:18:04Z

xml/System.Net/IPAddress.xml

+ If the length of `address` is 4, this method constructs an IPv4 address; otherwise, an IPv6 address with a scope of 0 is constructed.  
+
+ The byte span is assumed to be in network byte order with the most significant byte first in index position 0.  


Should these be documented in the param rather than remarks?

+1
This is an interesting suggestion. We should always keep in mind that remarks do not show up in intellisense, so if we add this information at the end of the param, we ensure developers have this information available first hand.

That's one reason, but also these remarks are all specific to the one param.

scalablecory · 2019-10-04T18:21:25Z

xml/System.Net/IPAddress.xml

-        <summary>To be added.</summary>
-        <returns>To be added.</returns>
+        <param name="ipString">A character span that contains an IP address in dotted-quad notation for IPv4 and in colon-hexadecimal notation for IPv6.</param>
+        <summary>Converts an IP address represented as a character span to an IPAddress instance.</summary>


Do we use the language "converts" elsewhere in docs when we are parsing a string?

@scalablecory It is used in the other method signature, also it is used in IPEndPoint.Parse and basically in any other type, int in example

scalablecory · 2019-10-04T18:22:48Z

xml/System.Net/IPAddress.xml

-        <param name="bytesWritten">To be added.</param>
-        <summary>To be added.</summary>
-        <returns>To be added.</returns>
+        <param name="destination">When this method returns, the IP address as a span of bytes.</param>


Suggested change

<param name="destination">When this method returns, the IP address as a span of bytes.</param>

<param name="destination">When this method returns <see langword="true" />, receives the byte representation of the IP address in network byte order.</param>

I agree but shouldn't it be "contains" instead of "receives"? I also agree on the "When this method returns <see langword="true" />" part but that is not the pattern that other TryParse APIs follow.

"contains" to me is a little confusing because it sounds like a precondition to calling this method. "receives" tells me it will have the IP address written to it.

@mairaw thoughts on true vs langword true?

I don't think we need to say contains. So what happens when the method returns false?

@mairaw if TryWriteBytes return false, it means that it didn't write into destination.

thoughts on true vs langword true?

@scalablecory Your suggestion is correct: we should use <see langword="true" /> instead of just true.

+1 What @mairaw said: We should mention what the contents of destination will be if the method returns false.

I'd be okay leaving without true or false as well. I just wouldn't say
"When this method returns true, ..."

I'd simply say "When this method returns, "

People should be checking the result of the method anyway before trying to use these, I'd say

scalablecory · 2019-10-04T18:23:09Z

xml/System.Net/IPAddress.xml

-        <summary>To be added.</summary>
-        <returns>To be added.</returns>
+        <param name="destination">When this method returns, the IP address as a span of bytes.</param>
+        <param name="bytesWritten">When this method returns, the number of bytes written into the span.</param>


Suggested change

<param name="bytesWritten">When this method returns, the number of bytes written into the span.</param>

<param name="bytesWritten">When this method returns <see langword="true" />, receives the number of bytes written into the span.</param>

I don't think this suggestion is correct. bytesWritten will always have a number, whether the method returns true or false. Usually these methods set this argument to 0 right before they return false. If that's the case, there's no point in mentioning "When this method returns true|false".

What @jozkee wrote might be enough, if he can confirm that the method sets this to zero on failure.

I prefer the original version better. So if it's simply 0 when false, the original sentence still looks good.

Co-Authored-By: Maira Wenzel <[email protected]> Co-Authored-By: Cory Nelson <[email protected]>

carlossanlop · 2019-11-21T23:31:55Z

xml/System.Net/IPAddress.xml

-        <summary>To be added.</summary>
-        <returns>To be added.</returns>
+        <param name="ipString">The byte span to validate.</param>
+        <param name="address">When this method returns, the <see cref="T:System.Net.IPAddress" /> version of the byte span.</param>


What happens to address if the method returns false?

it is unchanged.

mairaw

LGTM. There are only a few comments to address related to the out params.

carlossanlop

@jozkee this looks good to merge, and the build passed without warnings.
Feel free to address or ignore this suggestion. You can also address it separately in a new PR.

jozkee

Addressing...

Added documentation for APIs in System.Net namespace introduced in 2.x

2648b07

jozkee requested a review from karelz as a code owner October 2, 2019 00:16

carlossanlop requested review from mairaw, rpetrusha, davidsh, wfurt, scalablecory, ManickaP, eiriktsarpalis and carlossanlop October 2, 2019 16:21

carlossanlop assigned jozkee Oct 2, 2019

carlossanlop added 🏁 Release: .NET Core 2.x Identifies work items for the .NET Core 2.x releases new-content Indicates PRs that contain new articles waiting-on-reviews Indicates PRs that cannot be merged because of the lack of reviews labels Oct 2, 2019

carlossanlop added this to the October 2019 milestone Oct 2, 2019

carlossanlop reviewed Oct 2, 2019

View reviewed changes

xml/System.Net/IPAddress.xml Show resolved Hide resolved

mairaw requested changes Oct 3, 2019

View reviewed changes

scalablecory approved these changes Oct 4, 2019

View reviewed changes

Apply some suggestions from code review

c0d7a49

Co-Authored-By: Maira Wenzel <[email protected]> Co-Authored-By: Cory Nelson <[email protected]>

jozkee requested review from mairaw and removed request for rpetrusha November 21, 2019 01:08

carlossanlop reviewed Nov 21, 2019

View reviewed changes

avoid CI edit

592b8db

mairaw approved these changes Nov 22, 2019

View reviewed changes

carlossanlop approved these changes Nov 23, 2019

View reviewed changes

jozkee commented Nov 23, 2019

View reviewed changes

Address moving paragraph from remarks to param tag

e274e0c

jozkee merged commit d2e9568 into dotnet:master Nov 23, 2019

jozkee deleted the 2.x/System.Net branch November 23, 2019 01:08

mairaw removed the waiting-on-reviews Indicates PRs that cannot be merged because of the lack of reviews label Dec 5, 2019

		If the length of `address` is 4, this method constructs an IPv4 address; otherwise, an IPv6 address with a scope of 0 is constructed.

		The byte span is assumed to be in network byte order with the most significant byte first in index position 0.

	<param name="destination">When this method returns, the IP address as a span of bytes.</param>
	<param name="destination">When this method returns <see langword="true" />, receives the byte representation of the IP address in network byte order.</param>

	<param name="bytesWritten">When this method returns, the number of bytes written into the span.</param>
	<param name="bytesWritten">When this method returns <see langword="true" />, receives the number of bytes written into the span.</param>

Added documentation for APIs in System.Net namespace introduced in 2.x #3280

Added documentation for APIs in System.Net namespace introduced in 2.x #3280

Uh oh!

Conversation

jozkee commented Oct 2, 2019

Uh oh!

jozkee commented Oct 2, 2019

Uh oh!

Uh oh!

mairaw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

scalablecory left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scalablecory Oct 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jozkee Oct 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlossanlop Nov 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mairaw left a comment

Choose a reason for hiding this comment

Uh oh!

carlossanlop left a comment

Choose a reason for hiding this comment

Uh oh!

jozkee left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

scalablecory Oct 4, 2019 •

edited

Loading

jozkee Oct 10, 2019 •

edited

Loading

carlossanlop Nov 21, 2019 •

edited

Loading

jozkee left a comment •

edited

Loading