Denormalize required values to nodes #914
Conversation
JamesNK
commented
Nov 17, 2018
•
JamesNK
commented
- Matching
- Link generation
- Fix MvcEndpointDataSource tests
src/Microsoft.AspNetCore.Routing/Patterns/DefaultRoutePatternTransformer.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.AspNetCore.Routing/CompositeEndpointDataSource.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.AspNetCore.Routing/Internal/RequiredValueHelpers.cs
Outdated
Show resolved
Hide resolved
JamesNK
changed the title
|
Matching is passing! |
| } | ||
| } | ||
|
|
||
| AddLiteralNode(includeLabel, nextParents, parent, requiredValue.ToString()); |
There was a problem hiding this comment.
In a case like Foo/{action=Index} where rv: { action = "Index" } how does Foo match? Is that handled by the handling for default values? This is worth a comment at least.
There was a problem hiding this comment.
Is that handled by the handling for default values?
Yes, HasAdditionalRequiredSegments will add the endpoint as a match for the current node. I don't think this needs a comment to call that out here. There is no difference between how that logic interacts with required values and anything else.
I'll add a comment about the what is happening to the required value before it is used as a literal.
test/Microsoft.AspNetCore.Routing.Tests/Matching/DfaMatcherBuilderTest.cs
Show resolved
Hide resolved
|
🆙 📅 |
JamesNK
changed the title
|
We're merging Routing today into aspnet/AspNetCore. If you can merge this in the next 30 minutes or so, we can bring this along. Otherwise, you'll have to open a new PR against https://github.com/aspnet/AspNetCore. |
| // Copyright (c) .NET Foundation. All rights reserved. | ||
| // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
|
||
|
|
There was a problem hiding this comment.
extra line. According to people like you this is very important
There was a problem hiding this comment.
Thank you, this is important 🙏
test/Microsoft.AspNetCore.Routing.Tests/Template/TemplateBinderTests.cs
Outdated
Show resolved
Hide resolved
|
FYI @ryanbrandenburg this will likely be the last change to master before I make this repo readonly. |
|
This requires a reaction in MVC tho |
|
Will be rebased. Close this 💀 |
|
Just chatted with James. Yes, this will need to be rebased onto aspnet/AspNetCore. I'm going to close this PR, but I'll make sure to leave this branch around so you can copy the code over. |
