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

Clarify meaning of the group name specified in WithGroupName #59784

Merged
merged 1 commit into from
Jan 28, 2025

Conversation

mikekistler
Copy link
Contributor

This PR makes a small change to the description of the value passed to WithGroupName to explain how it is used in OpenAPI document generation. The prior text was inaccurate because there is no "GroupName" in the OpenAPI specification.

@mikekistler mikekistler requested a review from halter73 as a code owner January 9, 2025 03:05
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Jan 9, 2025
@@ -115,7 +115,8 @@ public static TBuilder WithName<TBuilder>(this TBuilder builder, string endpoint
/// Sets the <see cref="EndpointGroupNameAttribute"/> for all endpoints produced
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This also seems confusing -- what does it mean to "set an attribute"?

Maybe this should say "Sets the IEndpointGroupNameMetadata" ? or ???

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think this doc comment is attempting to apply a literal description of what this method is doing. It instantiates the EndpointGroupNameAttribute class and adds it to the metadata collection.

However, that's a means to an end for the real valuable thing which is the fact that EndpointGroupNameAttribute implements the IEndpointGroupNameMetadata interface. That's what's actually relevant here so I think your recommendation of referencing the interface instead of the class is a good one.

@mikekistler mikekistler force-pushed the mdk/clarify-groupname branch from 794c900 to 2854b64 Compare January 12, 2025 13:46
@mikekistler mikekistler force-pushed the mdk/clarify-groupname branch from 2854b64 to b0c117e Compare January 12, 2025 14:08
Copy link
Member

@captainsafia captainsafia left a comment

Choose a reason for hiding this comment

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

LGTM!

Maybe we can put the detail about group name mapping to the OpenAPI document in the docs for IEndpointGroupNameMetadata itself?

@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Jan 20, 2025
@lewing lewing merged commit a9daf61 into main Jan 28, 2025
27 checks passed
@lewing lewing deleted the mdk/clarify-groupname branch January 28, 2025 22:18
@dotnet-policy-service dotnet-policy-service bot added this to the 10.0-preview2 milestone Jan 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants