Create custom collections for several MVC types#6459
Create custom collections for several MVC types#6459henkmollema wants to merge 7 commits intoaspnet:devfrom henkmollema:henk/6161
Conversation
| /// as a wrapper for the specified list. | ||
| /// </summary> | ||
| /// <param name="modelBinderProviders">The list that is wrapped by the new collection.</param> | ||
| public ModelBinderProviderCollection(IList<IModelBinderProvider> modelBinderProviders) |
There was a problem hiding this comment.
I've implemented these constructors according to FormatterCollection, do we always need them?
There was a problem hiding this comment.
It's good to have for completeness any time you're extending Collection<>
| /// <summary> | ||
| /// Represents a collection of model binder providers. | ||
| /// </summary> | ||
| public class ModelBinderProviderCollection : Collection<IModelBinderProvider> |
There was a problem hiding this comment.
This type lives in Microsoft.AspNetCore.Mvc.Core. Is that correct? I noticed that some collection types are in Abstractions and some in Core.
| /// Removes all model binder providers of the specified type. | ||
| /// </summary> | ||
| /// <param name="modelBinderType">The type to remove.</param> | ||
| public void RemoveType(Type modelBinderType) |
There was a problem hiding this comment.
I've implemented this like
. Except it usesGetType() rather than a type check, is that OK?
There was a problem hiding this comment.
Should we use two different implementations? One generic and one non generic?
There was a problem hiding this comment.
ah I see the issue, yeah we should change all of this code (FormatterCollection and FilterCollection to use GetType() == for comparison.
|
@rynowak this is a draft of the design for #6161. I've left some comments, could you take a look and tell me what you think? |
|
Thanks for the feedback! I'll try to finish this tomorrow. |
|
@rynowak I think that's all of them. Is the amount of duplicate acceptable? And how should we update the breaking changes file to reflect these changes? |
rynowak
left a comment
There was a problem hiding this comment.
This looks great! Thanks again
|
@pranavkm can you take a look at this as well |
pranavkm
left a comment
There was a problem hiding this comment.
Looks good. I can get this merged in and add these methods to PageConventionCollection
| /// Removes all application model conventions of the specified type. | ||
| /// </summary> | ||
| /// <typeparam name="TApplicationModelConvention">The type to remove.</typeparam> | ||
| public void RemoveType<TApplicationModelConvention>() where TApplicationModelConvention : IApplicationModelConvention |
There was a problem hiding this comment.
I should port these over to Pages as well.
|
Ok thanks, please do the needful. Thanks again @henkmollema |
|
Cool! Let me know if you need me to do anything. |
|
Merged in 948982e. Thanks @henkmollema |
Resolves #6161.
FormatterCollectionto include a non-genericRemoveType(Type)overload.