Add documentation to System.Net by FilipToth · Pull Request #57855 · dotnet/runtime

FilipToth · 2021-08-20T18:02:56Z

This PR adds missing documentation to the HeaderEncodingSelector, HttpContent, SocketsHttpHandler, NegotiateStream and WebHeaderCollection types.

TODO:

~~P:System.Net.WebHeaderCollection.Item(System.String)~~
T:System.Net.Http.HeaderEncodingSelector`1
M:System.Net.Http.HttpContent.CreateContentReadStream(System.Threading.CancellationToken)
P:System.Net.Http.SocketsHttpHandler.Properties
M:System.Net.Security.NegotiateStream.WriteAsync(System.ReadOnlyMemory{System.Byte},System.Threading.CancellationToken)
M:System.Net.Security.NegotiateStream.WriteAsync(System.Byte[],System.Int32,System.Int32,System.Threading.CancellationToken)

Contributes to #43859

ghost · 2021-08-20T18:03:05Z

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

This PR adds missing documentation to the HeaderEncodingSelector, HttpContent, HttpRequestMessage, HttpRequestOptions, and WebHeaderCollection types.

Items that still need to be addressed:

M:System.Net.Http.HttpRequestOptions.#ctor`
M:System.Net.Http.HttpRequestOptionsKey`1.#ctor(System.String)
T:System.Net.Http.HttpRequestOptionsKey`1
P:System.Net.Http.HttpRequestOptionsKey`1.Key
P:System.Net.Http.SocketsHttpHandler.Properties
M:System.Net.Security.NegotiateStream.WriteAsync(System.ReadOnlyMemory{System.Byte},System.Threading.CancellationToken)
M:System.Net.Security.NegotiateStream.WriteAsync(System.Byte[],System.Int32,System.Int32,System.Threading.CancellationToken)

Fixes: #43859

Author:	FilipToth
Assignees:	-
Labels:	`area-System.Net.Http`
Milestone:	-

dnfadmin · 2021-08-20T18:03:09Z

All CLA requirements met.

And fix some badly formed XML...

geoffkizer · 2021-08-23T01:25:54Z

Adding @carlossanlop because he understands this stuff best.

Some of this stuff, at least, has documentation already; e.g. https://docs.microsoft.com/en-us/dotnet/api/system.net.http.socketshttphandler.cookiecontainer?view=net-5.0

It's not in the doc comments in the source, but it exists out there somewhere that produces content like the link above.

@carlossanlop Can you comment here? What is the right thing to be doing for stuff like this?

FilipToth · 2021-08-23T05:14:11Z

Adding @carlossanlop because he understands this stuff best.

I still don't think this is ready to be merged. I'll make this into a regular PR when I feel like the documentation is descriptive enough and compliant with all documentation standards. But I'd love to hear everyone's opinion on this addition and maybe what to refine. As I already mentioned, this is not the final thing.

Some of this stuff, at least, has documentation already; e.g. https://docs.microsoft.com/en-us/dotnet/api/system.net.http.socketshttphandler.cookiecontainer?view=net-5.0
It's not in the doc comments in the source, but it exists out there somewhere that produces content like the link above.

Yes, that is something I'll have to look into as it's planned to extract API documentation from the source code (#44969).
I just added the documentation as said here #43859. For example, some methods were said (in the table in issue #43859) to have a summary and parameter descriptions but weren't in the XML comments, but I found the documentation on MS Docs. This is a little messy as we essentially have two places for documentation. Should I also copy the missing XML Documentation from MS Docs into XML comments, just to be more consistent?

ManickaP

@carlossanlop do we care about porting back an existing documentation to triple slash? If so, shouldn't we do that automatically?

Also, some properties/methods have more details in the docs, e.g. PooledConnectionIdleTimeout#Property Value. Should that be ported as well?

karelz · 2021-08-23T12:40:55Z

@ManickaP there was effort to port docs back into triple-slash comments. We didn't act on it for 6.0, I assume it will be brought up again soon.

ManickaP · 2021-08-23T17:07:47Z

there was effort to port docs back into triple-slash comments. We didn't act on it for 6.0, I assume it will be brought up again soon.

I better oriented myself in the original issue. Porting back docs to triple slash is not the goal of #43859. Which is what I see in this PR.

@FilipToth Only really missing pieces of documentation should be added. The table in the original issue says precisely what's missing. Please let me know if you still want to tackle this? Otherwise, I'll put up a PR on my own.

ManickaP · 2021-08-23T17:08:44Z

And BTW the first item should be skipped since that API was removed in #37949

FilipToth · 2021-08-24T00:50:24Z

Only really missing pieces of documentation should be added.

Ok, I'll go back and check all added documentation to see if it already exists.

Please let me know if you still want to tackle this? Otherwise, I'll put up a PR on my own.

Yes, I definitely still want to do this.

And BTW the first item should be skipped since that API was removed in #37949

Ok, Thank you.

…into System.Net-Docs

FilipToth · 2021-08-30T18:04:43Z

Ok, marking this as ready for review as I think this should be OK.
Can someone add @carlossanlop as a reviewer, I don't have the privileges to do so.

ManickaP

Thank you for the contribution!

There are still some things in the table that are missing, e.g.:M:System.Net.Security.NegotiateStream.WriteAsync(System.ReadOnlyMemory{System.Byte},System.Threading.CancellationToken) etc.

So they either need to get added in this PR. Or the work can be split and this PR made only contributing to the original issue. Up to you @FilipToth.

ManickaP · 2021-09-01T08:07:06Z

@@ -10,6 +10,7 @@ namespace System.Net.Http
    /// </summary>
    /// <param name="headerName">Name of the header to specify the <see cref="Encoding"/> for.</param>
    /// <param name="context">The <typeparamref name="TContext"/> we are enoding/decoding the headers for.</param>


There's a typo one line above, could you please fix that as well:

Suggested change

/// <param name="context">The <typeparamref name="TContext"/> we are enoding/decoding the headers for.</param>

/// <param name="context">The <typeparamref name="TContext"/> we are encoding/decoding the headers for.</param>

ManickaP · 2021-09-01T08:24:08Z

        public IDictionary<string, object?> Properties => Options;

+        /// <summary>
+        /// Gets the options for the HTTP request.


I'd expect this to get a bit more specific description so that users actually know what it's good for a and how to use it.
But the whole Options related docs are WASM addition so it's up them how much details they want to put in, cc @lewing.

FilipToth · 2021-09-01T10:13:53Z

@ManickaP Thank you so much for the feedback. I'll be back with the requested changes.
As for the options, I can make it a bit more descriptive and we'll see if the WASM team thinks it's appropriate. If they won't, I'll remove it.

FilipToth · 2021-09-02T11:18:58Z

OK, I've made the changes as requested in the review. But I'm not sure about the HttpRequestMessage.Options property. This is the summary I came up with: Gets the special flags to use for the HTTP request. It's used to configure the HTTP request.. I am not sure if it's sufficient, but if not, I can change it.

ManickaP

Still missing summary for P:System.Net.Http.SocketsHttpHandler.Properties based on the orig issue table.

ManickaP · 2021-09-06T13:41:46Z

            return WriteAsync(new AsyncReadWriteAdapter(InnerStream, cancellationToken), new ReadOnlyMemory<byte>(buffer, offset, count));
        }

+        /// <returns>A <see cref="ValueTask"/> of void.</returns>


Suggested change

/// <returns>A <see cref="ValueTask"/> of void.</returns>

/// <returns>A <see cref="ValueTask"/> that represents the asynchronous read operation.</returns>

ManickaP · 2021-09-06T13:42:43Z

            return new ValueTask(WriteAsync(new AsyncReadWriteAdapter(InnerStream, cancellationToken), buffer));
        }

+        /// <returns>A <see cref="Task"/> of void.</returns>


This is a private method, not an API.

ManickaP · 2021-09-06T13:44:42Z

@lewing Hi, could someone review the Options part?

ManickaP · 2021-09-07T18:11:44Z

Hi @FilipToth, I just learned we need to get this in more urgently then we originally thought. So if you don't mind, I'll take over this PR.
I'll remove all the Options related docs, since we really need deeper explanations and that needs to be done by the team that did the change. I'll ping them offline and let them fill it separately. And I'll merge this PR as a contribution to the original issues instead of a full fix.
I hope you don't mind. We still appreciate your contribution and involvement, so thank you!

lewing · 2021-09-07T18:42:19Z

@pranavkm can you help review the options related work?

FilipToth · 2021-09-07T19:01:52Z

Hi @FilipToth, I just learned we need to get this in more urgently then we originally thought. So if you don't mind, I'll take over this PR.

Of course. I'm sorry, I just learned about the September 14th deadline. Makes sense to give you full control over the PR as this needs to be done fast. Thank you very much for your involvement with this community PR.

* Ported docs from dotnet/runtime#57855 * Update xml/System.Net.Http/HeaderEncodingSelector`1.xml Co-authored-by: Carlos Sanchez <1175054+carlossanlop@users.noreply.github.com>

FilipToth added 8 commits August 19, 2021 18:10

Add documentation to WebHeaderCollection

5668570

Add type parameter description to HeaderEncodingSelector<TContext>

8d18587

Add documentation for CreateContentReadStream method

2ad9439

Add summary to Options property of HttpRequestMessage class

b845ac5

Add summary for HttpRequestOptions class

20da612

Add documentation for Set<TValue> method of HttpRequestOptions class

6c5b8c1

Change wording of Set<TValue> method summary

651ea56

Add documentation for TryGetValue method of HttpRequestOptions class

a866640

ghost added area-System.Net.Http community-contribution Indicates that the PR has been added by a community member labels Aug 20, 2021

FilipToth added 9 commits August 20, 2021 14:36

Fix trailing whitespaces in XML comments

239f510

Remove trailing whitespaces

0bb7613

Add summary to HttpRequestOptionsKey

963136a

Add documentation HttpRequestOptionsKey

a7e7e3d

Add documentation to NegotiateStream.WriteAsync methods

7197240

And fix some badly formed XML...

Fix cref attribute

246f412

Add documentation to \BrowserHttpHandler\SocketsHttpHandler.cs

1c747a4

Add documentation to SocketsHttpHandler properties

9da47bb

Better documentation for WriteAsync methods

ec22eeb

ManickaP reviewed Aug 23, 2021

View reviewed changes

runfoapp Bot mentioned this pull request Aug 23, 2021

Can_Run_App_With_StatiHost installer test failure in CI #42101

Closed

karelz assigned ManickaP Aug 24, 2021

FilipToth added 3 commits August 24, 2021 20:56

Merge branch 'System.Net-Docs' of https://github.com/FilipToth/runtime …

086d28b

…into System.Net-Docs

Minor tweaks and fix test failures

8190cdd

Add some removed documentation

a6397b5

FilipToth marked this pull request as ready for review August 30, 2021 15:38

ManickaP reviewed Sep 1, 2021

View reviewed changes

Make changes according to review

64e13aa

FilipToth requested a review from ManickaP September 4, 2021 12:12

ManickaP reviewed Sep 6, 2021

View reviewed changes

ManickaP requested a review from lewing September 6, 2021 13:44

karelz added this to the 7.0.0 milestone Sep 7, 2021

FilipToth added 2 commits September 7, 2021 18:03

Make changes according to review

38721b6

Add documentation to SocketsHttpHandler.CookieContainer

d856605

ManickaP removed the request for review from lewing September 8, 2021 07:48

Removed options docs, cleaned up the rest

a193171

ManickaP force-pushed the System.Net-Docs branch from 3f4a50d to a193171 Compare September 8, 2021 07:50

Updated Properties summary

845d160

ManickaP approved these changes Sep 8, 2021

View reviewed changes

ManickaP merged commit baf6353 into dotnet:main Sep 8, 2021

runfoapp Bot mentioned this pull request Sep 8, 2021

RunContinueWithStressTestsNoState timing out in CI #2271

Closed

ManickaP added a commit to ManickaP/dotnet-api-docs that referenced this pull request Sep 8, 2021

Ported docs from dotnet/runtime#57855

a0dc465

FilipToth deleted the System.Net-Docs branch September 10, 2021 06:01

ghost locked as resolved and limited conversation to collaborators Nov 7, 2021

	/// <param name="context">The <typeparamref name="TContext"/> we are enoding/decoding the headers for.</param>
	/// <param name="context">The <typeparamref name="TContext"/> we are encoding/decoding the headers for.</param>

	/// <returns>A <see cref="ValueTask"/> of void.</returns>
	/// <returns>A <see cref="ValueTask"/> that represents the asynchronous read operation.</returns>

Uh oh!

Conversation

FilipToth commented Aug 20, 2021 • edited by ManickaP Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODO:

Uh oh!

ghost commented Aug 20, 2021

Uh oh!

dnfadmin commented Aug 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

geoffkizer commented Aug 23, 2021

Uh oh!

FilipToth commented Aug 23, 2021

Uh oh!

ManickaP left a comment

Choose a reason for hiding this comment

Uh oh!

karelz commented Aug 23, 2021

Uh oh!

ManickaP commented Aug 23, 2021

Uh oh!

ManickaP commented Aug 23, 2021

Uh oh!

FilipToth commented Aug 24, 2021

Uh oh!

FilipToth commented Aug 30, 2021

Uh oh!

ManickaP left a comment

Choose a reason for hiding this comment

Uh oh!

ManickaP Sep 1, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ManickaP Sep 1, 2021

Choose a reason for hiding this comment

Uh oh!

FilipToth commented Sep 1, 2021

Uh oh!

FilipToth commented Sep 2, 2021

Uh oh!

ManickaP left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ManickaP Sep 6, 2021

Choose a reason for hiding this comment

Uh oh!

FilipToth Sep 6, 2021 • 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!

ManickaP Sep 6, 2021

Choose a reason for hiding this comment

Uh oh!

ManickaP commented Sep 6, 2021

Uh oh!

ManickaP commented Sep 7, 2021

Uh oh!

lewing commented Sep 7, 2021

Uh oh!

FilipToth commented Sep 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

FilipToth commented Aug 20, 2021 •

edited by ManickaP

Loading

dnfadmin commented Aug 20, 2021 •

edited

Loading

FilipToth Sep 6, 2021 •

edited

Loading

FilipToth commented Sep 7, 2021 •

edited

Loading