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

Add useNormalisedPathByDefault to Router #536

Open
wants to merge 1 commit into
base: master
from

Conversation

@yunyu
Copy link
Contributor

commented Mar 1, 2017

This lets users disable normalised paths on a router without applying it to every single route. Fixes #469. Needs tests.

@vietj vietj added the to review label Mar 1, 2017
Copy link
Member

left a comment

It does require some small updates plus a couple of simple tests, validating the behavior with and without.

The tricky part here is how to handle subrouters, should the properly propagate or not? why?

@@ -168,7 +168,7 @@

/**
* If true then the normalised request path will be used when routing (e.g. removing duplicate /)
* Default is true
* Defaults to useNormalisedPathByDefault in the router

This comment has been minimized.

Copy link
@pmlopes

pmlopes Mar 2, 2017

Member

You should make a reference to the Router#useNormalisedPath class so readers can follow

This comment has been minimized.

Copy link
@yunyu
* @return a reference to this, so the API can be used fluently
*/
@Fluent
Router useNormalisedPathByDefault(boolean useNormalisedPathByDefault);

This comment has been minimized.

Copy link
@pmlopes

pmlopes Mar 2, 2017

Member

I think useNormalisedPath would be a better name WDYT?

This comment has been minimized.

Copy link
@yunyu

yunyu Mar 2, 2017

Author Contributor

I added byDefault because useNormalisedPath may imply that it will completely override the behavior for all routes, and there should be a distinction. I can change it if you really think it's good to share names.

@yunyu yunyu force-pushed the yunyu:router-use-normalise-path-default branch from 56526ce to 57a1ccd Mar 2, 2017
@yunyu

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2017

After thinking a bit more, I'm leaning towards not propagating to sub-routers and just adopting the value specified in the sub-router. Here's some reasons as to why:

  • Developers refactoring top-level routers into sub-routers may find that their behavior has broken if it propagates, this is the most important one
  • There's no concept of the "default" state necessary for proper propagation, and incorporating one would result in breakages and/or inconsistencies, it's easier and more flexible just to have sub-routers override
  • The additional line required to force propagation to sub-routers by the developer is a trivial barrier that prevents mistakes

I have added some tests for this updated behavior.

@yunyu yunyu force-pushed the yunyu:router-use-normalise-path-default branch 4 times, most recently from 242ea51 to 6ca2f29 Mar 2, 2017
This lets users disable normalised paths on a router without applying it to every single route. Sub-routers override this option.

Signed-off-by: Yunyu Lin <yunyu.lin@vanderbilt.edu>
@yunyu yunyu force-pushed the yunyu:router-use-normalise-path-default branch from 6ca2f29 to 4a04ca5 Mar 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.