Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RP conventions topic+sample updates #8523

Merged
merged 3 commits into from
Sep 17, 2018
Merged

Conversation

guardrex
Copy link
Collaborator

@guardrex guardrex commented Sep 13, 2018

Fixes #6173

Internal Review Topic

@rynowak This follows our discussion on aspnet/Mvc#8385.

Thanks again to @user135711 for surfacing the issues.

@guardrex guardrex added the WIP label Sep 13, 2018
@guardrex guardrex force-pushed the guardrex/rp-conventions2 branch from 22ba661 to 60109ec Compare September 14, 2018 18:01
@guardrex guardrex force-pushed the guardrex/rp-conventions2 branch from 9430fd6 to d23e1ed Compare September 14, 2018 18:23
@guardrex guardrex removed the WIP label Sep 14, 2018
@guardrex guardrex changed the title [WIP] RP conventions topic+sample updates RP conventions topic+sample updates Sep 14, 2018
@guardrex guardrex requested a review from rynowak September 14, 2018 18:31
* When routes have the same `Order`, the most specific route is matched first followed by less specific routes.
* When routes with the same `Order` and the same number of parameters match a request URL, routes are processed in the order that they're added to the <xref:Microsoft.AspNetCore.Mvc.ApplicationModels.PageConventionCollection>.

If possible, avoid depending on an established route processing order. Generally, routing selects the correct route with URL matching. If you must set route `Order` properties to route requests correctly, the app's routing scheme is probably confusing to clients and fragile to maintain. Seek to simplify the app's routing scheme. The sample app requires an explicit route processing order to demonstrate several routing scenarios using a single app. However, you should attempt to avoid the practice of setting route `Order` in production apps.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section all seems pretty good. If there's details about this in the regular routing section it might be helpful to link it. This is a shared implementation with regular routing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, there's one spot in routing where this is discussed, and there is some duplication. Of course, PageCollectionConvention isn't discussed over there. I'll take another look at this in a few minutes and see about cross-linking both ways (just in case someone looking for RP content lands over there).

@guardrex guardrex requested a review from scottaddie September 17, 2018 18:05
Copy link
Member

@scottaddie scottaddie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update the ms.date value on each of the Markdown files you touched.

@guardrex guardrex merged commit b7ba934 into master Sep 17, 2018
@guardrex guardrex deleted the guardrex/rp-conventions2 branch September 17, 2018 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants