The Wayback Machine - https://web.archive.org/web/20220209225401/https://github.com/dotnet/aspnetcore/pull/40106
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix nearly all CS1591 #40106

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Fix nearly all CS1591 #40106

wants to merge 3 commits into from

Conversation

@pranavkm
Copy link
Contributor

@pranavkm pranavkm commented Feb 9, 2022

Fixes #27538

@pranavkm
Copy link
Contributor Author

@pranavkm pranavkm commented Feb 9, 2022

Bonus 💯 points if you find the doc bug.

src/Razor/Razor/src/TagHelpers/HtmlAttributeValueStyle.cs Outdated Show resolved Hide resolved
Tcp,

/// <summary>
/// Carries water when not burst.
Copy link
Member

@BrennanConroy BrennanConroy Feb 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:/

Http3 = 0x4,

/// <summary>
/// The <see cref="Http1"/>, <see cref="Http2"/> and <see cref="Http3"/> protocol versions.
Copy link
Member

@sharwell sharwell Feb 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// The <see cref="Http1"/>, <see cref="Http2"/> and <see cref="Http3"/> protocol versions.
/// The <see cref="Http1"/>, <see cref="Http2"/>, and <see cref="Http3"/> protocol versions.

/// <summary>
/// A binary transport format.
/// </summary>
Binary = 0x01,

/// <summary>
/// A text output format.
/// </summary>
Text = 0x02
Copy link
Member

@sharwell sharwell Feb 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 Would prefer to see consistent word structure for these two values.

/// <summary>
/// This API supports framework infrastructure and is not intended to be used
/// directly from application code.
/// </summary>
Copy link
Member

@sharwell sharwell Feb 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Consider using <inheritdoc /> for this

Suggested change
/// <summary>
/// This API supports framework infrastructure and is not intended to be used
/// directly from application code.
/// </summary>
/// <inheritdoc cref="HttpMethod" />

Copy link
Member

@sharwell sharwell Feb 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also create a type specifically for this purpose, and then link all documentation to it for elements to have consistent wording.

/// <summary>
/// Specify Quic as the transport to be used by Kestrel.
/// </summary>
/// <param name="hostBuilder">The <see cref="IWebHostBuilder"/>. to configure.</param>
Copy link
Member

@sharwell sharwell Feb 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// <param name="hostBuilder">The <see cref="IWebHostBuilder"/>. to configure.</param>
/// <param name="hostBuilder">The <see cref="IWebHostBuilder"/> to configure.</param>

/// Specify Quic as the transport to be used by Kestrel.
/// </summary>
/// <param name="hostBuilder">The <see cref="IWebHostBuilder"/>. to configure.</param>
/// <returns>The <see cref="IWebHostBuilder"/>.</returns>
Copy link
Member

@sharwell sharwell Feb 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💭 Maybe make this directly reference the parameter to show that it's a specific instance and not just an arbitrary instance of the type?

Suggested change
/// <returns>The <see cref="IWebHostBuilder"/>.</returns>
/// <returns>Returns <paramref name="hostBuilder"/>.</returns>

/// <summary>
/// Specify Quic as the transport to be used by Kestrel.
/// </summary>
/// <param name="hostBuilder">The <see cref="IWebHostBuilder"/>. to configure.</param>
Copy link
Member

@sharwell sharwell Feb 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// <param name="hostBuilder">The <see cref="IWebHostBuilder"/>. to configure.</param>
/// <param name="hostBuilder">The <see cref="IWebHostBuilder"/> to configure.</param>

@@ -16,6 +16,11 @@ public sealed class SocketTransportFactory : IConnectionListenerFactory
private readonly SocketTransportOptions _options;
private readonly ILoggerFactory _logger;

/// <summary>
/// Initializes a new instance of <see cref="SocketTransportFactory"/>.
Copy link
Member

@sharwell sharwell Feb 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Initializes a new instance of <see cref="SocketTransportFactory"/>.
/// Initializes a new instance of the <see cref="SocketTransportFactory"/> class.

/// <summary>
/// Not a manual.
/// </summary>
Auto,

/// <summary>
/// Better than UDP.
/// </summary>
Tcp,

/// <summary>
/// Carries water when not burst.
/// </summary>
Pipe
Copy link
Member

@sharwell sharwell Feb 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe want to revisit these

/// <summary>
/// Better than UDP.
/// </summary>
Copy link
Member

@Tratcher Tratcher Feb 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Take that QUIC

@@ -9,10 +9,33 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core;
[Flags]
public enum HttpProtocols
{
/// <summary>
/// A missing HTTP protocol version.
Copy link
Member

@Tratcher Tratcher Feb 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// A missing HTTP protocol version.
/// No HTTP protocol version was specified.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants