-
Notifications
You must be signed in to change notification settings - Fork 25.1k
Minimal API responses.md: add missing IBindableFromHttpContext section #35999
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
base: main
Are you sure you want to change the base?
Conversation
Fixed typo on code link
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.
I haven't reviewed carefully, but I think at mininum the content needs to move to a different section. Let's work that out first and then flag me to re-review.
@@ -274,6 +274,32 @@ An alternative approach is using the <xref:Microsoft.AspNetCore.Mvc.ProducesAttr | |||
|
|||
:::code language="csharp" source="~/fundamentals/minimal-apis/9.0-samples/Snippets/Program.cs" id="snippet_11"::: | |||
|
|||
## Custom parameter binding with `IBindableFromHttpContext` |
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.
This topic is showing up in an article titled "Create responses", but it is about parameter binding. I think this is in the wrong place -- it should appear within the article with the title "Parameter binding".
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.
Thanks @mikekistler, I moved the new section to the Parameter-binding.md topic, and updated the intro for Custom Binding to include it here: https://review.learn.microsoft.com/en-us/aspnet/core/fundamentals/minimal-apis/parameter-binding?view=aspnetcore-10.0&branch=pr-en-us-35999#custom-binding
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.
Do we need 4 different sample apps and 4 separate include files for this? I don't think the feature has changed substantially since ASP.NET Core 7 so I think we only need one app and feature description.
Also I think that describing this as "for advanced scenarios" is a bit vague. I think the point of this interface is to avoid the need for reflection and therefore to allow the app to be AOT compatible. If that's correct, I think we should state that explicitly.
Fixes #32903
This was missed when it became available clear back in v7. (I did not own that at the time, but jumping in to fix it)
For versions 7-10 of the parameter-binding.md topic:
Custom parameter binding with IBindableFromHttpContext
Internal previews