Add Controller for multiple areas sample #262
Conversation
|
@jbagga, |
| ControllerModels = new List<ControllerModel>(); | ||
| } | ||
|
|
||
| public IList<ControllerModel> ControllerModels { get; set; } |
There was a problem hiding this comment.
This should be a local variable, there's no need for it to exist outside the scope of Apply
| @@ -0,0 +1,34 @@ | |||
| using System; | |||
There was a problem hiding this comment.
You seem to have two copies of the files:
samples/Mvc.ControllerForMultipleAreasSample/Startup.cs
samples/Mvc/Mvc.ControllerForMultipleAreasSample/Startup.cs
|
|
||
| <ItemGroup> | ||
| <PackageReference Include="Microsoft.AspNetCore.All" Version="2.0.0" /> | ||
| </ItemGroup> |
There was a problem hiding this comment.
This should look like our other projects in this repo, and use the latest bits
| foreach (var controller in application.Controllers) | ||
| { | ||
| var areas = controller.ControllerType.GetCustomAttributes<MultipleAreasAttribute>().FirstOrDefault(); | ||
| var areaNames = areas?.AreaNames; |
There was a problem hiding this comment.
it would be clearer if you just short circuited this entirely
|
|
||
| namespace ControllerForMultipleAreasSample | ||
| { | ||
| public class MultipleAreasControllerConvention : IApplicationModelConvention |
There was a problem hiding this comment.
You can also use IControllerModelConvention instead and then you don't need the loop.
There was a problem hiding this comment.
How will the new copied controllers with different area value in its RouteValues be added to the application?
There was a problem hiding this comment.
Not explained here but we ran into a problem with this approach. You can't modify application.Controllers while inside an IControllerModelConvention.
| [AttributeUsage(AttributeTargets.Class | AttributeTargets.Method, AllowMultiple = false, Inherited = true)] | ||
| public class MultipleAreasAttribute : Attribute | ||
| { | ||
| public MultipleAreasAttribute(string[] areaNames) |
There was a problem hiding this comment.
a handy pattern for these things is to do something like MulipleAreasAttribute(string a1, string a2, params string[] a3...) that makes it clear you're supposed to use at least 2 values.
| var areas = controller.ControllerType.GetCustomAttributes<MultipleAreasAttribute>().FirstOrDefault(); | ||
| var areaNames = areas?.AreaNames; | ||
|
|
||
| for (var i = 0; i < areaNames?.Length; i++) |
There was a problem hiding this comment.
a more common pattern for this is to just start the loop at 1. So move L28 outside of the loop, and then simplify the body to just L33-35
|
|
||
| namespace ControllerForMultipleAreasSample | ||
| { | ||
| [AttributeUsage(AttributeTargets.Class | AttributeTargets.Method, AllowMultiple = false, Inherited = true)] |
There was a problem hiding this comment.
I think this attribute can target only classes, right? Not methods?
| AreaNames = areaNames; | ||
| } | ||
|
|
||
| public string[] AreaNames { get; private set; } |
There was a problem hiding this comment.
Probably don't need private set since this is set only in the ctor.
| @@ -0,0 +1,13 @@ | |||
| using Microsoft.AspNetCore.Mvc; | |||
There was a problem hiding this comment.
Add license headers to all the files too.
jbagga
force-pushed
the
jbagga/MultipleAreasControllerSample
branch
from
af3176b to
aa43965
Compare
|
|
||
| using Microsoft.AspNetCore.Mvc; | ||
|
|
||
| namespace ControllerForMultipleAreasSample.Areas.Products.Controllers |
There was a problem hiding this comment.
It might be clearer to put this somewhere else. This could just go in the root (file and namespace wise)
| @@ -0,0 +1,31 @@ | |||
| using System.Collections.Generic; | |||
| @@ -0,0 +1 @@ | |||
| Services View | |||
There was a problem hiding this comment.
|
@Eilon Updated |
There was a problem hiding this comment.
Just a few tiny comments, plus I suggest adding a README.md to the folder of this sample that has a few short words about what this is (and even link back to the original issue that motivated creating this sample).
Here are a few examples of reasonably meaningful readmes for inspiration:
| public MultipleAreasAttribute(string area1, string area2, params string[] areaNames) | ||
| { | ||
| AreaNames = new string[] { area1, area2 }; | ||
| AreaNames = AreaNames.Concat(areaNames).ToArray(); |
There was a problem hiding this comment.
Can probably do both the above lines in 1 line of code.
|
|
||
| foreach (var model in controllerModels) | ||
| { | ||
| application.Controllers.Add(model); |
There was a problem hiding this comment.
Any reason not to do this within the inner nested loop above? That is, instead of making a list, then iterating over the list, why not just add each new model immediately?
There was a problem hiding this comment.
Because that modifies the collection (application.Controllers), which halts the enumeration (in the above for loop).
|
@Eilon Added a README |
Sample code for aspnet/Mvc#6637
cc @rynowak @Eilon