Support for controller-level attribute routes. Resolves #44 #45
Conversation
|
Thank you; this is looking good 👍 Did you accidentally check in |
|
Oops, yes apparently I did. Please reject the pull request so that I can correct my branch and re-submit it. |
| } | ||
|
|
||
| var newRouteValues = AddControllerNameAndMethodToRouteValues(callExpression, routeValues); | ||
| newRouteValues = RemoveControllerAndActionRouteValuesIfNeeded(controllerRouteAttribute, newRouteValues); |
There was a problem hiding this comment.
It's not that I want to be a killjoy here, but I can remove this method call without any tests failing.
It's not that I predict that a lot of work will be done on Hyprlinkr in the future, but I tend to rely heavily on unit tests when I refactor. My modus operandi is that if all tests are still green after a refactoring, then I broke nothing.
There's a risk, then, that I (or someone else) might break a feature for you in the future, because this isn't covered by tests. If this is important to you, would it be possible to add some tests? Or perhaps remove this for now, and then come back to it in a later pull request?
| var newRouteValues = new Dictionary<string, object>(routeValues); | ||
| var controllerName = callExpression.Object.Type.Name.ToLowerInvariant().Replace("controller", ""); | ||
| newRouteValues["controller"] = controllerName; | ||
| newRouteValues["action"] = callExpression.Method.Name.ToLowerInvariant(); |
There was a problem hiding this comment.
I can remove the above three statements without breaking any tests. What value do these lines of code add?
There was a problem hiding this comment.
No problem, I should have added additional test cases.
Those methods manage the "controller" and "action" KVPs which may or may not be necessary to create the actual route Uri, depending on the actual Route Template.
The problem is that if they are put into the RouteValues dictionary and they are not needed, they will show up incorrectly as querystrings, which is not desirable.
I'll write the test cases that illustrate these issues.
There was a problem hiding this comment.
Here's a suggestion, which will make things go smoother:
- Decline this pull request. You can do this yourself.
- Send a new pull request without these extra features, but only handling of Controller-level attributes.
- Send another pull request later with the other changes.
Making pull requests smaller tend to make the entire process faster.
The
DefaultRouteDispatcherwas modified to check the controller for aRouteattribute, similarly to what it does with actions.As per MSDN documentation
Routeattributes on actions take precedence overRouteattributes on controllers.If no attribute is found, the
DefaultRouteDispatcherworks as before.A new test was created to check this feature (
DispatchReturnsResultWithRouterAttributeControllerRouteName).