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

fix(UrlMatcherFactory): Correct trailing slash of terminal optional parameter #2198

Merged
merged 1 commit into from
Aug 25, 2015

Conversation

JakobJingleheimer
Copy link
Contributor

Fixes #1902

@christopherthielen
Copy link
Contributor

Thanks jshado1

Does this perform symmetrically, i.e., does it also match properly when the trailing slashes are omitted?
Also, I don't think this obeys the $urlMatcherFactory.strictMode setting.

Can you address those issues and push an updated PR?

I'd like to see additional unit tests for more combinations, with the above questions in mind accounting for:

  • optional and non-optional
  • strictMode setting
  • URL generation symmetric with matching

@christopherthielen christopherthielen added this to the 0.2.16 milestone Aug 24, 2015
@christopherthielen christopherthielen self-assigned this Aug 24, 2015
@@ -342,6 +342,8 @@ UrlMatcher.prototype.format = function (values) {

if (isPathParam) {
var nextSegment = segments[i + 1];
var isFinalPathParam = i + 1 >= nPath;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this should probably be changed to i + 1 === nPath (isPathParam already ensures that this is within nPath).

@JakobJingleheimer
Copy link
Contributor Author

Hi @christopherthielen,

I think there're other tests that (inadvertently?) assert cases 1 and 2. I'll look for them.

Is it safe to assume that param.squash will inherit defaultSquashPolicy when it hasn't been explicitly set?

Thanks for the feedback.

@nateabele
Copy link
Contributor

Also, I don't think this obeys the $urlMatcherFactory.strictMode setting.

That should really only apply to parsing.

Is it safe to assume that param.squash will inherit defaultSquashPolicy when it hasn't been explicitly set?

https://github.com/jshado1/ui-router/blob/1902/src/urlMatcherFactory.js#L202

@jshado1 If you wanna make the change you noted above and squash your commits, this is 👍 from me.

Thanks for your work on this!

@JakobJingleheimer
Copy link
Contributor Author

@christopherthielen, non-optional is covered by urlMatcherFactorySpec.js#L617.

Does this perform symmetrically, i.e., does it also match properly when the trailing slashes are omitted?

Possibly lacking caffeine, but what is "it" ?

@nateabele will do when i get home tonight.

@JakobJingleheimer
Copy link
Contributor Author

@nateabele I made that change I had commented on (and squashed).

@christopherthielen
Copy link
Contributor

Regarding my symmetry comment, I want to make sure that if strict mode is on and a url is generated (with trailing slash removed), that the url will then be matched correctly to the same state and paeans (for instance if the user hits reload in the browser)

nateabele added a commit that referenced this pull request Aug 25, 2015
fix(UrlMatcherFactory): Correct trailing slash of terminal optional parameter
@nateabele nateabele merged commit f92d3dd into angular-ui:master Aug 25, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants