SignalR additional xml docs #7980
Conversation
| @@ -985,11 +1010,17 @@ private async Task ReceiveLoop(ConnectionState connectionState) | |||
| } | |||
| } | |||
|
|
|||
| /// <summary> | |||
| /// This method is for internal framework use and should not be called by user code. | |||
There was a problem hiding this comment.
Or just make it private :)
There was a problem hiding this comment.
I didn't want to bog this PR down with that, but I can just remove the doc comment. It was public in 2.2 so our standard policy is to deprecate it over one release first. See #7977 for more. I'll remove these changes and we can just discuss things there.
src/SignalR/common/Http.Connections/src/HttpConnectionContextExtensions.cs
Outdated
Show resolved
Hide resolved
| /// </summary> | ||
| /// <remarks> | ||
| /// If this event was triggered from a connection error, the <see cref="Exception"/> that occurred will be passed in as the | ||
| /// sole argument to this handler. If this event was triggered from a call to <see cref="StopAsync(CancellationToken)"/>, then |
There was a problem hiding this comment.
You might want to add the server closing the connection gracefully case here as well
| /// <remarks> | ||
| /// SignalR connections can run on top of HTTP transports like WebSockets or Long Polling, or other non-HTTP transports. As a result, | ||
| /// this method can sometimes return <see langword="null"/> depending on the configuration of your application. | ||
| /// </remarks> |
There was a problem hiding this comment.
Since this is a remark, beware that this won't show up in intellisense or the HttpConnectionContextExtensions generated docs. To see this you will have to got to the generated docs page dedicated exclusively to the GetHttpContext() method.
There was a problem hiding this comment.
Yeah, I think I'll put or <see langword="null" /> if there is no HTTP context in the summary (and leave this remark that adds more detail).
analogrelay
force-pushed
the
anurse/signalr-minor-xml-docs
branch
from
7db7b39 to
22e706c
Compare
|
https://github.com/aspnet/AspNetCore/issues/7265 - which should now be skipped. Rerunning |
|
/azp run AspNetCore-helix-test |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| /// } | ||
| /// else | ||
| /// { | ||
| /// Console.WriteLine($"Connection closed due to an error: {exception.Message}"); |
There was a problem hiding this comment.
Nit: I normally prefer printing
Reviewed our xml docs as both ramping back up after leave and docathon. Things look pretty good, added a few things to some public methods.