T4MVC / T4MVC

T4MVC is a T4 template for ASP.NET MVC apps that creates strongly typed helpers that eliminate the use of literal strings in many places.
Apache License 2.0
304 stars 74 forks source link

Routing in abstract controllers #6

Open migig opened 9 years ago

migig commented 9 years ago

Looks like the recent commit is incomplete, it doesn't break anything but it also doesn't work as expected.

Abstract controllers are processed by T4MVC as expected, but they don't route as expected because of this code in the FooController.generated.cs:

[GeneratedCode("T4MVC", "2.0"), DebuggerNonUserCode]
public partial class T4MVC_FooController : My.Namespace.Controllers.FooController
{
    public T4MVC_FooController() : base(Dummy.Instance) { }

    [NonAction]
    partial void IndexOverride(T4MVC_System_Web_Mvc_ActionResult callInfo);

    [NonAction]
    public override System.Web.Mvc.ActionResult Index()
    {
        var callInfo = new T4MVC_System_Web_Mvc_ActionResult(Area, Name, ActionNames.Index);
        IndexOverride(callInfo);
        return callInfo;
    }

It's not my code, and the codebase has zero comments, so it's hard to tell why. I'm not sure if it's because it overrides the base version and then declares [NonAction], but removing that doesn't solve the problem.

I don't know why it worked for me before, but since then we've updated the MVC references to newer versions, maybe that's why?

The good thing is that unlike before, you can process an abstract controller and not get tons of compiler warnings.

migig commented 9 years ago

My suggesion is to rollback to the previous version, even though nothing breaks I don't want to pullute T4MVC with code that doesnt work as expected for the abstract case.

So

i.e. this should stay (as well as the corresponding re-enable line):

    // 0108: suppress "Foo hides inherited member Foo. Use the new keyword if hiding was intended." when a controller and its abstract parent are both processed
   #pragma warning disable 1591, 3008, 3009, 0108
davidebbo commented 9 years ago

Just catching up on this. Do you mean that the change doesn't work at all in any scenario, or only in the Abstract controllers scenario? If the latter, then it seems unnecessary to revert the entire change.

migig commented 9 years ago

Yes you're right... Routing seems to work for the "weirdly named controllers" case. Just not for the abstract case.

So: I''' remove the abstract case for now, and keep the pragmas that solve that long-running compiler warning issue about the abstract case.

Which commit can I base the changes on? It's just that quite a few commit were made when moving to github so I don't know which to use?

davidebbo commented 9 years ago

You can just rebase whatever changes you have on the latest commit in the master branch.

migig commented 9 years ago

Only got a chance to look at it now... Oh no this is what I was afraid of! Not a git user, and it's giving me errors, because there have been commits since mine (I was hoping to "rollback" but its not possible now).

The only change needed is from this:

    if (!HasControllerAttribute(type))
        target.IsAbstract = true;
    // ...unless it has the [T4MVC] attribute, then process all the action methods in the controller
    else
        ProcessControllerActionMethods(target, type);

to this (the original code before I changed it):

    target.IsAbstract = true;

I will keep trying though. All my svn skills are useless :)