Improve documentation RequestHeaders and ResponseHeaders #14971

koenekelschot · 2019-10-13T20:57:30Z

Summary of the changes (Less than 80 chars)

Added xmldocs

Addresses #3465

dnfclas · 2019-10-13T20:57:43Z

All CLA requirements met.

jkotalik · 2019-10-14T04:08:33Z

@anurse your favorite kind of PR 😄.

jkotalik · 2019-10-14T04:09:45Z

src/Http/Http.Extensions/src/RequestHeaders.cs

+        /// <typeparam name="T">The type of the header.
+        /// The given type must have a static TryParse method</typeparam>
+        /// <param name="name">The name of the header to retrieve.</param>
+        /// <returns>The value of the header</returns>


As a follow-up, we should either make another PR or add to this PR to add xml documentation for the rest of these properties.

Also, isn't there an analyzer that we can enable to check for all public apis that don't have xml docs?

One small nit:

Suggested change

/// <returns>The value of the header</returns>

/// <returns>The value of the header.</returns>

Also, isn't there an analyzer that we can enable to check for all public apis that don't have xml docs?

It's even better than that :). If we turn the right settings on we get a build warning (or error if we promote it to that). Some projects are using that now. We need to get "clean" before we can turn that on more broadly though.

@jkotalik and @anurse thanks for the comments! They should be resolved in the latest commit.

Regarding the other properties in these classes, I'm willing to document them as well, but do add those docs to this PR or do I open a new PR?

analogrelay

Generally looks good! Let's make all the methods consistent in how the docs are formatted. Once we've got that, I think we'll be good to go.

analogrelay · 2019-10-14T18:51:47Z

src/Http/Http.Extensions/src/RequestHeaders.cs

+        /// <typeparam name="T">The type of the header.
+        /// The given type must have a static TryParse method</typeparam>
+        /// <param name="name">The name of the header to retrieve.</param>
+        /// <returns>The value of the header</returns>


One small nit:

Suggested change

/// <returns>The value of the header</returns>

/// <returns>The value of the header.</returns>

Also, isn't there an analyzer that we can enable to check for all public apis that don't have xml docs?

It's even better than that :). If we turn the right settings on we get a build warning (or error if we promote it to that). Some projects are using that now. We need to get "clean" before we can turn that on more broadly though.

analogrelay · 2019-10-14T18:51:57Z

src/Http/Http.Extensions/src/RequestHeaders.cs

+        /// <typeparam name="T">The type of the header.
+        /// The given type must have a static TryParseList method</typeparam>
+        /// <param name="name">The name of the header to retrieve.</param>
+        /// <returns>List of values of the header</returns>


Suggested change

/// <returns>List of values of the header</returns>

/// <returns>List of values of the header.</returns>

analogrelay · 2019-10-14T18:52:39Z

src/Http/Http.Extensions/src/RequestHeaders.cs

+        /// <summary>
+        /// Gets the values of header with <paramref name="name"/>.
+        /// </summary>
+        /// <remarks><typeparamref name="T"/> must contain a TryParseList method with the signature <code>public static bool TryParseList(IList&lt;string&gt;, out IList&lt;T&gt;)</code></remarks>


You used typeparamref here but not in the Get method. I'm fine with doing this either way but we should be consistent.

Similarly, you put the full signature in here, but not above.

We should also be consistent between remarks and typeparam items. Let's make them similar in format and consistent between both methods.

analogrelay · 2019-10-14T18:53:38Z

src/Http/Http.Extensions/src/ResponseHeaders.cs

+        /// <summary>
+        /// Gets the values of header with <paramref name="name"/>.
+        /// </summary>
+        /// <remarks><typeparamref name="T"/> must contain a TryParseList method with the signature <code>public static bool TryParseList(IList&lt;string&gt;, out IList&lt;T&gt;)</code></remarks>


Same comments about consistency apply here.

Tratcher · 2019-10-15T16:18:55Z

@aspnet-hello triage https://dev.azure.com/dnceng/public/_build/results?buildId=387600

aspnet-hello · 2019-10-15T16:22:00Z

This comment was made automatically. If there is a problem contact [email protected].

I've triaged the above build. I've created/commented on the following issue(s)
https://github.com/aspnet/AspNetCore-Internal/issues/3241
https://github.com/aspnet/AspNetCore-Internal/issues/3128
https://github.com/aspnet/AspNetCore-Internal/issues/3173

Tratcher · 2019-10-15T21:41:31Z

@aspnet-hello triage https://dev.azure.com/dnceng/public/_build/results?buildId=387600

Tratcher · 2019-10-15T21:44:35Z

@HaoK Build Tests: Helix x64 still looks too flaky to be required.

HaoK · 2019-10-15T22:31:54Z

Looks like there might be something going on with redis causing these tests to fail:

   System.AggregateException : One or more errors occurred. (Command '/usr/bin/docker run --rm -p 6379:6379 --name redisTestContainer -d redis' failed with exit code '125'. Output:
Unable to find image 'redis:latest' locally
/usr/bin/docker: Error response from daemon: received unexpected HTTP status: 503 Service Unavailable.
See '/usr/bin/docker run --help'.

Tratcher · 2019-10-16T16:41:34Z

Thanks

Improve documentation RequestHeaders and ResponseHeaders

6bd8355

koenekelschot requested review from analogrelay, jkotalik and Tratcher as code owners October 13, 2019 20:57

jkotalik reviewed Oct 14, 2019

View reviewed changes

jkotalik added the area-servers label Oct 14, 2019

analogrelay added this to the 5.0.0-preview1 milestone Oct 14, 2019

analogrelay suggested changes Oct 14, 2019

View reviewed changes

improve consistency

b33f775

Tratcher approved these changes Oct 15, 2019

View reviewed changes

analogrelay approved these changes Oct 15, 2019

View reviewed changes

Tratcher merged commit 1599a5a into dotnet:master Oct 16, 2019

amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 24, 2023

	/// <returns>The value of the header</returns>
	/// <returns>The value of the header.</returns>

	/// <returns>List of values of the header</returns>
	/// <returns>List of values of the header.</returns>

Improve documentation RequestHeaders and ResponseHeaders #14971

Improve documentation RequestHeaders and ResponseHeaders #14971

Uh oh!

Conversation

koenekelschot commented Oct 13, 2019

Uh oh!

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

Uh oh!

Uh oh!

jkotalik commented Oct 14, 2019

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!

analogrelay left a comment

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!

Tratcher commented Oct 15, 2019

Uh oh!

aspnet-hello commented Oct 15, 2019

Uh oh!

Tratcher commented Oct 15, 2019

Uh oh!

Tratcher commented Oct 15, 2019

Uh oh!

HaoK commented Oct 15, 2019

Uh oh!

Tratcher commented Oct 16, 2019

Uh oh!

Uh oh!

dnfclas commented Oct 13, 2019 •

edited

Loading