-
Notifications
You must be signed in to change notification settings - Fork 25.1k
Add ProducesResponseType Description documentation #35009
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
Add ProducesResponseType Description documentation #35009
Conversation
The "What's new in .NET 10" release notes mention the new Description property I added, but the .NET 10 OpenAPI docs do not. This commit adds consistency
|
I've had issues with the |
|
Sidenote: .NET 10 Preview 1's When I click on the |
|
Hello @sander1095 ... The build warnings this morning are appearing on all of our PRs and are unrelated to what you're submitting. A member of our team will file an internal bug report on them. |
|
@captainsafia ... No giant rush, just making sure that you saw the ping on this one. |
|
I added a very quick pass to update code spacing and a few other NITs 😈. |
|
Nevermind! ... I see it now. It's in a different file ... |
|
Hi @guardrex ! Are there other things I can do to get this merged? :) |
guardrex
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks fine with a few more tweaks.
The main things are ...
- Use a serial comma, also called an Oxford comma, in lists.
- Shorten code lines to no more than 85 characters for narrow screens. We try to avoid horizontal scrollbars on our code examples.
|
@captainsafia ... Do you want to look this over? I see there are some discussion points in the OP here. |
|
@captainsafia will need to review. @Rick-Anderson ... I don't think Safia is seeing these pings. Would you email her to let her know this is here? |
|
@guardrex Ah, I just pinged you as your status is still "requested changes" :) Wondering if there's still something I can do or not. If not, perhaps you should update that state :) |
|
I'm waiting for Safia to come on and hear from her if changes are required. If not, I'll take a final look, sign off, and merge. |
|
There's still a bug with this attribute for minimal APIs when @mikekistler last tried it. Change seems fine but let's hold back on merging until then. |
mikekistler
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a ton of whitespace changes in this PR that I hope we can avoid. This might be an editor setting or something but it would be best to avoid changing whitespace in places were there is no actual content change.
The actual content changes look fine to me and I'm happy to approve once the whitespace changes are fixed/minimized.
|
Hi @mikekistler . Good catch! Those whitespace changes were made by @guardrex , however. So I guess they are intended. My original commit doesn't contain any of the whitespace changes as far as I can see. @guardrex , can you comment on Mike's findings? |
|
Let's link this PR to the issue that still contains that bug: dotnet/aspnetcore#60518 (comment) . I've posted my findings in that issue and I'm waiting for Safia's feedback (or another teammember) on what direction we want to go with a the bug fix. |
|
Indeed, the whitespace changes that I applied are in keeping with our article style conventions. For example, we use four-space indents for C# code, and we have an 85-character line length limit for code lines to accommodate narrow screens.
@mikekistler ... Was there something specific that you wanted to ask about WRT the whitespace? I'm a walking encyclopedia of style manual rules! 😆 I'll double-check the whole article to make sure that everything is in order before merging this later. BTW ... If it becomes known which preview the bug fix will land, that's helpful for us to get this merged at the right time. Dan would like the preview PRs to hit right about the time (same day if possible) that the preview release comes out. |
mikekistler
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. 👍
Sorry ... I thought the whitespace changes were unintentional since I often make that mistake myself.
I don't feel strongly about holding these changes for the bug fix to land. The content correctly describes the desired behavior, even though it doesn't currently describe the actual behavior in all cases.
|
Can we add line in a good spot here that alerts devs to the issue? For example, something like ... |
|
@sander1095 ... See if you can place that line (or something like that with the link formatted that way) at a good spot. Then, we'll go ahead and get this merged after I take one final look 👀. |
|
It seems like the content of the Minimal API tab for UPDATE: I couldn't make it a suggestion on the DIFF because multiple unchanged lines couldn't be rolled into the suggestion. See if what I just did on the last commit works 👇. |
|
Hi @guardrex . My apologies, but I don't really understand the change you'd like to me to perform. Could you elaborate? |
|
I already made it. See the last commit ☝️. Does that make sense for the time being until the bug is fixed? UPDATE: This was blocking, so we went ahead and merged it. |
The "What's new in .NET 10" release notes mention the new Description property I added, but the .NET 10 OpenAPI docs do not.
This commit adds consistency.
I've added examples for
ProducesResponseTypeandProducesDefaultResponseTypeto Minimal API and Controller docs. I know a team member doesn't likeProducesDefault(@mikekistler, dotnet/aspnetcore#58719 (comment) ).I also know that another team member (@captainsafia , dotnet/aspnetcore#58724 (comment)) would prefer developers to use the endpoint-specific OpenAPI transformers (Which will be released in a future preview) instead of setting the
Descriptionfor Minimal API's, as they are more extensible.Therefore, I'd like to discuss the following:
ProducesDefault? This way, we make it clear that it IS possible by mentioning it's existance, but also "discourage" developers from using it by not showing any explicit code examples.Descriptionproperty. However, when Support registering OpenApiOperationTransformer via extension method for Minimal APIs aspnetcore#59180 lands in a future preview, I'd propose we update this page and mention that THAT approach is preferred over theDescriptionproperty in Minimal API scenario's.Fixes #33974
Fixes #35016
Internal previews