Skip to content
This repository has been archived by the owner on Dec 14, 2018. It is now read-only.

* Simplify MvcOptions #2328

Closed
wants to merge 1 commit into from
Closed

* Simplify MvcOptions #2328

wants to merge 1 commit into from

Conversation

pranavkm
Copy link
Contributor

@pranavkm pranavkm commented Apr 3, 2015

  • Remove facades for accessing Options and pass options to the invoker

Fixes #2266
Fixes #2269

@ghost ghost added the cla-not-required label Apr 3, 2015
var formatters = _bindingContext.Value.InputFormatters;
var requestServices = bindingContext.OperationBindingContext.HttpContext.RequestServices;

var formatterSelector = requestServices.GetRequiredService<IInputFormatterSelector>();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Started a thread - #2330 - to discuss if we could change the contract for formatters. It might allow us to get away from doing GetRequiredService here.

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems ugly to get all these 4 services as part of execution. Is this the general pattern for anything that needs scoped/instance services?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hopefully addressing the layering might solve this. We had a chat with @loudej and he thought this was ok. However I think there's an ongoing discussion \ work item about visiting the usage of GetRequiredServices in our framework, so this should get covered as part of it.

harshgMSFT added a commit that referenced this pull request Apr 3, 2015
The fix is to use the model name as the model state key if the model is null OR if there is an exception while deserializing.
Adding a model state error with empty key is not useful.
@rynowak
Copy link
Member

rynowak commented Apr 5, 2015

This is a really gigantic PR, is it possible to break up at this point?

@pranavkm
Copy link
Contributor Author

pranavkm commented Apr 5, 2015

@rynowak, let me give it a try. I can start by un-deleting the files to start with - perhaps it might make the diffs less daunting.

* Remove facades for accessing Options<T> and pass options to the invoker

Fixes #2266
Fixes #2269
@pranavkm
Copy link
Contributor Author

pranavkm commented Apr 5, 2015

Ok - Sending it out in parts - this contains most of the code changes without the file deletions. Think this should be manageable.

[NotNull] IValueProviderFactoryProvider valueProviderFactoryProvider,
[NotNull] IReadOnlyList<IModelBinder> modelBinders,
[NotNull] IReadOnlyList<IModelValidatorProvider> modelValidatorProviders,
[NotNull] IReadOnlyList<IValueProviderFactory> valueProviderFactories,
Copy link
Member

Choose a reason for hiding this comment

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

Bonus RyanPoints for using IReadOnlyList 😍

@rynowak
Copy link
Member

rynowak commented Apr 7, 2015

:shipit:

@pranavkm pranavkm closed this Apr 7, 2015
@pranavkm pranavkm deleted the OptionDescriptor branch April 7, 2015 21:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants