Skip to content

Conversation

@captainsafia
Copy link
Member

Closes #39226.

@ghost ghost added the area-runtime label Jan 13, 2022
@captainsafia captainsafia marked this pull request as ready for review January 14, 2022 18:45
/// <param name="configureOptions">The <see cref="Action{JsonOptions}"/> to configure the
/// <see cref="JsonOptions"/>.</param>
/// <returns>The modified <see cref="IServiceCollection"/>.</returns>
public static IServiceCollection ConfigureHttpJsonOptions(this IServiceCollection services, Action<JsonOptions> configureOptions)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should review this name.

Copy link
Member

Choose a reason for hiding this comment

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

Is this the first time we're adding a ConfigureWhatever() IServiceCollection extension method for an option type? I guess in most cases it wouldn't be necessary because you could have an overload that takes a configureOptions callback to AddWhatever() for most option types.

The only thing I see right now that's close to this is ConfigureApplicationCookie() and ConfigureExternalCookie() which use named options.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated the name to ConfigureRouteHandlerJsonOptions per our email conversation but open to other ideas.

@Kahbazi
Copy link
Member

Kahbazi commented Jan 15, 2022

Also Closes #35904

@halter73
Copy link
Member

@Kahbazi Do you not care about endpoint specific JSON options?

@Kahbazi
Copy link
Member

Kahbazi commented Jan 19, 2022

@Kahbazi Do you not care about endpoint specific JSON options?

@halter73 It's not an issue for me at the moment, since it's also doable by combination of BindAsync and Results.Json.
Also based on @davidfowl 's comment, I get the sense that that issue is limited to add a method to configure HttpJsonOptions.

@captainsafia captainsafia changed the title Add ConfigureHttpJsonOptions for M.A.Htt.Json.JsonOptions Add ConfigureRouteHandlerJsonOptions for M.A.Htt.Json.JsonOptions Jan 21, 2022
@captainsafia captainsafia merged commit 7e6674c into dotnet:main Jan 21, 2022
@ghost ghost added this to the 7.0-preview1 milestone Jan 21, 2022
ShreyasJejurkar pushed a commit to ShreyasJejurkar/aspnetcore that referenced this pull request Jan 22, 2022
…tnet#39502)

* Add ConfigureHttpJsonOptions for M.A.Htt.Json.JsonOptions

* Address initial feedback from API review

* Fix method reference in doc comment

* Fix up crefs
@captainsafia captainsafia deleted the safia/json-options branch February 4, 2022 22:50
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 24, 2023
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Consider a nicer API for configuring JsonOptions for minimal actions / route-to-code

7 participants