Added documentation for new System.Net APIs released in 3.0 #2663

jozkee · 2019-06-28T23:26:11Z

List of APIs documented:

CookieCollection:

M:System.Net.CookieCollection.Clear
M:System.Net.CookieCollection.Contains(System.Net.Cookie)
M:System.Net.CookieCollection.Remove(System.Net.Cookie)
M:System.Net.CookieCollection.System#Collections#Generic#IEnumerable<System#Net#Cookie>#GetEnumerator

FileWebRequest:

M:System.Net.FileWebRequest.GetRequestStreamAsync
M:System.Net.FileWebRequest.GetResponseAsync

IPEndPoint:

M:System.Net.IPEndPoint.Parse(System.ReadOnlySpan{System.Char})
M:System.Net.IPEndPoint.Parse(System.String)
M:System.Net.IPEndPoint.TryParse(System.String,System.Net.IPEndPoint@)
M:System.Net.IPEndPoint.TryParse(System.ReadOnlySpan{System.Char},System.Net.IPEndPoint@)

jozkee · 2019-06-28T23:33:02Z

@davidsh @wfurt @krwq @scalablecory since you are the System.Net owners, could you please take a look?

jozkee · 2019-06-28T23:39:50Z

@mairaw @rpetrusha

xml/System.Net/CookieCollection.xml

scalablecory · 2019-06-29T00:07:14Z

xml/System.Net/CookieCollection.xml

-        <remarks>To be added.</remarks>
+        <param name="cookie">The <see cref="T:System.Net.Cookie" /> to remove form the <see cref="T:System.Net.CookieCollection" />.</param>
+        <summary>Determines whether the <paramref name="cookie" /> is in the <see cref="T:System.Net.CookieCollection" />.</summary>
+        <returns><see langword="true" /> if item was successfully removed from the <see cref="T:System.Net.CookieCollection" />; otherwise, <see langword="false" />. This method also returns <see langword="false" /> if item is not found in the original collection.</returns>


if item is not found in the original collection

Was this copied from ICollection? There is no "item" parameter here.

scalablecory · 2019-06-29T00:10:20Z

xml/System.Net/FileWebRequest.xml

-        <returns>To be added.</returns>
-        <remarks>To be added.</remarks>
+        <summary>Returns a <see cref="T:System.IO.Stream" /> object for writing data to the file system resource as an asynchronous operation.</summary>
+        <returns>The Task object representing the asynchronous operation.</returns>


"Task" -> <see cref="...Task"/>.

@scalablecory @jozkee there is a neat feature to help provide code suggestions that can be bulk merged:

Suggested change

<returns>The Task object representing the asynchronous operation.</returns>

<returns>The <see cref="T:System.Threading.Tasks.Task" /> object representing the asynchronous operation.</returns>

Note: The comment is already outdated so you may ignore it.

scalablecory · 2019-06-29T00:11:47Z

xml/System.Net/IPEndPoint.xml

-        <summary>To be added.</summary>
-        <returns>To be added.</returns>
+        <param name="s">A <see cref="T:System.RedOnlySpan`1" /> that contains an IP endpoint in dotted-quad notation or network byte order for IPv4 and in colon-hexadecimal notation for IPv6.</param>
+        <summary>Converts an IP network endpoint (address and port) represented as a <see cref="T:System.RedOnlySpan`1" /> to an IPEndPoint instance.</summary>


RedOnlySpan -> ReadOnlySpan

Suggested change

<summary>Converts an IP network endpoint (address and port) represented as a <see cref="T:System.RedOnlySpan`1" /> to an IPEndPoint instance.</summary>

<summary>Converts an IP network endpoint (address and port) represented as a <see cref="T:System.ReadOnlySpan`1" /> to an <see cref="T:System.Net.IPEndPoint" /> instance.</summary>

scalablecory · 2019-06-29T00:12:19Z

xml/System.Net/IPEndPoint.xml

+        <param name="s">A <see cref="T:System.RedOnlySpan`1" /> that contains an IP endpoint in dotted-quad notation or network byte order for IPv4 and in colon-hexadecimal notation for IPv6.</param>
+        <summary>Converts an IP network endpoint (address and port) represented as a <see cref="T:System.RedOnlySpan`1" /> to an IPEndPoint instance.</summary>
+        <returns>An <see cref="T:System.Net.IPEndPoint" /> instance.</returns>
        <remarks>To be added.</remarks>


Remarks still todo?

Since overloads are generated in the same html, for this case and TryParse I think it would be redundant to have practically the same remarks twice.

Yes, that's good. But if they are applicable to both, the best would be to create a MemberGroup element and add the remarks there.

Here's an example I did this for:
6950528
https://docs.microsoft.com/en-us/dotnet/api/windows.foundation.point.tostring?view=dotnet-uwp-10.0

Otherwise, only one of the overloads have those remarks:
https://review.docs.microsoft.com/en-us/dotnet/api/system.net.ipendpoint.tryparse?view=netcore-3.0&branch=pr-en-us-2663#System_Net_IPEndPoint_TryParse_System_String_System_Net_IPEndPoint__

I can help you to format that in this PR if you'd like.

Remarks are optional, @scalablecory

scalablecory · 2019-06-29T00:15:32Z

xml/System.Net/FileWebRequest.xml

-        <returns>To be added.</returns>
-        <remarks>To be added.</remarks>
+        <summary>Returns a response to a file system request as an asynchronous operation.</summary>
+        <returns>The Task object representing the asynchronous operation.</returns>


"Task" -> <see cref="...Task"/>.

scalablecory · 2019-06-29T00:17:42Z

xml/System.Net/FileWebRequest.xml

+          <format type="text/markdown"><![CDATA[  
+          ## Remarks  
+          > [!NOTE]
+          >  In the case of asynchronous requests, it is the responsibility of the client application to implement its own time-out mechanism.


Should this be a link to something describing how to implement time-outs?

there is an example for HttpWebRequest.BeginGetResponse I could adapt it to FileWebRequest.GetResponseAsync in case is really needed.

Created issue #2672 to address this later.

mairaw · 2019-06-29T00:40:59Z

You can see when your changes on the staging site: https://review.docs.microsoft.com/en-us/dotnet/api/system.net.ipendpoint?view=netcore-3.0&branch=pr-en-us-2663

mairaw

Nice job documenting these @jozkee! Left a few comments for you.

xml/System.Net/CookieCollection.xml

mairaw · 2019-06-29T00:43:34Z

xml/System.Net/IPEndPoint.xml

-        <summary>To be added.</summary>
-        <returns>To be added.</returns>
+        <param name="s">A <see cref="T:System.RedOnlySpan`1" /> that contains an IP endpoint in dotted-quad notation or network byte order for IPv4 and in colon-hexadecimal notation for IPv6.</param>
+        <summary>Converts an IP network endpoint (address and port) represented as a <see cref="T:System.RedOnlySpan`1" /> to an IPEndPoint instance.</summary>


Suggested change

<summary>Converts an IP network endpoint (address and port) represented as a <see cref="T:System.RedOnlySpan`1" /> to an IPEndPoint instance.</summary>

<summary>Converts an IP network endpoint (address and port) represented as a <see cref="T:System.ReadOnlySpan`1" /> to an <see cref="T:System.Net.IPEndPoint" /> instance.</summary>

mairaw · 2019-06-29T00:47:13Z

xml/System.Net/IPEndPoint.xml

+        <param name="s">A <see cref="T:System.RedOnlySpan`1" /> that contains an IP endpoint in dotted-quad notation or network byte order for IPv4 and in colon-hexadecimal notation for IPv6.</param>
+        <summary>Converts an IP network endpoint (address and port) represented as a <see cref="T:System.RedOnlySpan`1" /> to an IPEndPoint instance.</summary>
+        <returns>An <see cref="T:System.Net.IPEndPoint" /> instance.</returns>
        <remarks>To be added.</remarks>


Yes, that's good. But if they are applicable to both, the best would be to create a MemberGroup element and add the remarks there.

Here's an example I did this for:
6950528
https://docs.microsoft.com/en-us/dotnet/api/windows.foundation.point.tostring?view=dotnet-uwp-10.0

Otherwise, only one of the overloads have those remarks:
https://review.docs.microsoft.com/en-us/dotnet/api/system.net.ipendpoint.tryparse?view=netcore-3.0&branch=pr-en-us-2663#System_Net_IPEndPoint_TryParse_System_String_System_Net_IPEndPoint__

I can help you to format that in this PR if you'd like.

xml/System.Net/IPEndPoint.xml

mairaw · 2019-06-29T00:54:04Z

xml/System.Net/IPEndPoint.xml

+## Remarks  
+The number of parts (each part is separated by a period) in the string s determines how the endpoint's IP address is constructed. A one part address is stored directly in the network address. A two part address, convenient for specifying a class A address, puts the leading part in the first byte and the trailing part in the right-most three bytes of the network address. A three part address, convenient for specifying a class B address, puts the first part in the first byte, the second part in the second byte, and the final part in the right-most two bytes of the network address. For Example:
+
+|Number of parts and example `s`|IPv4 address for IPEndPoint.Address|Port


mairaw · 2019-06-29T01:01:36Z

xml/System.Net/IPEndPoint.xml

+|2 -- "20.65535:23"|20.0.255.255|23|  
+|3 -- "128.1.2:443"|128.1.0.2|443|  
+
+Note that this method accepts as valid an string value that can be parsed as an Int64, and then treats that Int64 as the long value of an IP address in network byte order, similar to the way that the IPAddress constructor does. This means that this method returns true if the Int64 is parsed successfully, even if it represents an address that's not a valid IP address. For example, if s is "1", this method returns true even though "1" (or 0.0.0.1) is not a valid IP address and you might expect this method to return false. Fixing this bug would break existing apps, so the current behavior will not be changed. Your code can avoid this behavior by ensuring that it only uses this method to parse IP addresses in dotted-decimal format.


This seems like an important note.

Suggested change

Note that this method accepts as valid an string value that can be parsed as an Int64, and then treats that Int64 as the long value of an IP address in network byte order, similar to the way that the IPAddress constructor does. This means that this method returns true if the Int64 is parsed successfully, even if it represents an address that's not a valid IP address. For example, if s is "1", this method returns true even though "1" (or 0.0.0.1) is not a valid IP address and you might expect this method to return false. Fixing this bug would break existing apps, so the current behavior will not be changed. Your code can avoid this behavior by ensuring that it only uses this method to parse IP addresses in dotted-decimal format.

> [!IMPORTANT]

> Note that this method accepts as valid an string value that can be parsed as an <xref:System.Int64>, and then treats that <xref:System.Int64> as the long value of an IP address in network byte order, similar to the way that the <xref:System.Net.IPAddress> constructor does. This means that this method returns `true` when the <xref:System.Int64> is parsed successfully, even if it represents an address that's not a valid IP address. For example, if `s` is "1", this method returns `true` even though "1" (or 0.0.0.1) is not a valid IP address. Fixing this bug would break existing apps, so the current behavior will not be changed. Your code can avoid this behavior by ensuring that it only uses this method to parse IP addresses in dotted-decimal format.

xml/System.Net/IPEndPoint.xml

davidsh · 2019-06-29T16:38:36Z

All of these types and methods aren't really new. What's new is that there are some overloads with ReadOnlySpan instead of other buffer types.

So, I would advise looking at that existing documentation to re-use most of the same descriptions of parameters, remarks, return values, etc. We only need small changes when new parameter types such as the ReadOnlySpan are added.

xml/System.Net/IPEndPoint.xml

jozkee · 2019-07-19T16:34:14Z

This is ready for re-review:

Added missing dot.
Removed AssemblyInfo from MemberGroups.
As per Added documentation for System.Net.Sockets APIs targeted for 3.0 #2767 (comment), first exception conditions should be written in the same line than exception tag.

@mairaw @rpetrusha

jozkee · 2019-07-31T22:38:14Z

@mairaw, @rpetrusha, Its been a while since I've addressed the comments on this one, can we merge it?

mairaw · 2019-08-06T22:02:18Z

Taking a look now @jozkee!

mairaw · 2019-08-06T22:49:38Z

@jozkee I've made some changes to the three files. Can you make sure I didn't introduce anything wrong? Also let me know if you dislike any of the changes I made. Thanks!

jozkee

I added a few comments regarding CookieCollection.GetEnumerator, other than that, your changes looks pretty good.
Also, nice catch on CookieCollection.Remove

xml/System.Net/CookieCollection.xml

jozkee · 2019-08-07T18:00:19Z

@mairaw Looks good to me, can we merge?

carlossanlop · 2019-08-08T16:28:00Z

@mairaw thanks for your last commit to fix the broken cref. The latest build passed without warnings. Is this good to merge?

carlossanlop · 2019-08-08T21:30:58Z

@mairaw successful build with no warnings. I think it's ready to merge.

Added documentation for new APIs released in 3.0

624cc2b

jozkee requested a review from karelz as a code owner June 28, 2019 23:26

mairaw assigned jozkee Jun 28, 2019

mairaw added new-content Indicates PRs that contain new articles 🏁 Release: .NET Core 3.0 :checkered_flag: Release: .NET Core 3.0 labels Jun 28, 2019

mairaw added this to the June 2019 milestone Jun 28, 2019

scalablecory reviewed Jun 29, 2019

View reviewed changes

mairaw requested changes Jun 29, 2019

View reviewed changes

davidsh added the area-System.Net label Jun 29, 2019

Addressing PR comments.

0f5457e

carlossanlop reviewed Jul 15, 2019

View reviewed changes

xml/System.Net/IPEndPoint.xml Outdated Show resolved Hide resolved

carlossanlop reviewed Jul 15, 2019

View reviewed changes

xml/System.Net/IPEndPoint.xml Outdated Show resolved Hide resolved

Addressed more PR comments.

01de167

mairaw modified the milestones: June 2019, July 2019 Jul 26, 2019

jozkee mentioned this pull request Aug 2, 2019

Document System.Numerics APIs #2912

Merged

mairaw added 3 commits August 6, 2019 15:19

Update CookieCollection.xml

254ba7a

Update FileWebRequest.xml

00ddc18

Update IPEndPoint.xml

be3c4b3

mairaw added the verify-build-before-merge label Aug 6, 2019

mairaw added the waiting-on-feedback Indicates PRs that are waiting for feedback from SMEs before they can be merged label Aug 6, 2019

jozkee commented Aug 7, 2019

View reviewed changes

xml/System.Net/CookieCollection.xml Outdated Show resolved Hide resolved

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

fixed cref

182d6b8

carlossanlop requested a review from mairaw August 8, 2019 21:30

carlossanlop added changes-addressed Indicates PRs that had all comments addressed and are awaiting for new review and removed waiting-on-feedback Indicates PRs that are waiting for feedback from SMEs before they can be merged labels Aug 8, 2019

mairaw removed changes-addressed Indicates PRs that had all comments addressed and are awaiting for new review verify-build-before-merge labels Aug 8, 2019

mairaw approved these changes Aug 8, 2019

View reviewed changes

mairaw merged commit 3d7e35f into dotnet:master Aug 8, 2019

	<returns>The Task object representing the asynchronous operation.</returns>
	<returns>The <see cref="T:System.Threading.Tasks.Task" /> object representing the asynchronous operation.</returns>

	<summary>Converts an IP network endpoint (address and port) represented as a <see cref="T:System.RedOnlySpan`1" /> to an IPEndPoint instance.</summary>
	<summary>Converts an IP network endpoint (address and port) represented as a <see cref="T:System.ReadOnlySpan`1" /> to an <see cref="T:System.Net.IPEndPoint" /> instance.</summary>

	\|Number of parts and example `s`\|IPv4 address for IPEndPoint.Address\|Port
	\|Number of parts and example `s`\|IPv4 address for IPEndPoint.Address\|Port\|

	Note that this method accepts as valid an string value that can be parsed as an Int64, and then treats that Int64 as the long value of an IP address in network byte order, similar to the way that the IPAddress constructor does. This means that this method returns true if the Int64 is parsed successfully, even if it represents an address that's not a valid IP address. For example, if s is "1", this method returns true even though "1" (or 0.0.0.1) is not a valid IP address and you might expect this method to return false. Fixing this bug would break existing apps, so the current behavior will not be changed. Your code can avoid this behavior by ensuring that it only uses this method to parse IP addresses in dotted-decimal format.
	> [!IMPORTANT]
	> Note that this method accepts as valid an string value that can be parsed as an <xref:System.Int64>, and then treats that <xref:System.Int64> as the long value of an IP address in network byte order, similar to the way that the <xref:System.Net.IPAddress> constructor does. This means that this method returns `true` when the <xref:System.Int64> is parsed successfully, even if it represents an address that's not a valid IP address. For example, if `s` is "1", this method returns `true` even though "1" (or 0.0.0.1) is not a valid IP address. Fixing this bug would break existing apps, so the current behavior will not be changed. Your code can avoid this behavior by ensuring that it only uses this method to parse IP addresses in dotted-decimal format.

Added documentation for new System.Net APIs released in 3.0 #2663

Added documentation for new System.Net APIs released in 3.0 #2663

Uh oh!

Conversation

jozkee commented Jun 28, 2019

Uh oh!

jozkee commented Jun 28, 2019

Uh oh!

jozkee commented Jun 28, 2019

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!

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!

mairaw commented Jun 29, 2019

Uh oh!

mairaw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

davidsh commented Jun 29, 2019

Uh oh!

Uh oh!

Uh oh!

jozkee commented Jul 19, 2019

Uh oh!

jozkee commented Jul 31, 2019

Uh oh!

mairaw commented Aug 6, 2019

Uh oh!

mairaw commented Aug 6, 2019

Uh oh!

jozkee left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jozkee commented Aug 7, 2019

Uh oh!

carlossanlop commented Aug 8, 2019

Uh oh!

carlossanlop commented Aug 8, 2019