Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix($route): fix redirection with optional/eager params #9827

Conversation

gkalpak
Copy link
Member

@gkalpak gkalpak commented Oct 29, 2014

Previously, when (automatically) redirecting from path that fetured a trailing slash and optional or "eager" parameters, the resulting path would (incorrectly) contain the special characters (?,*) along with the parameter values.
(There is also a first (unrelated) commit, that fixes the names of 2 tests in routeSpec.)

Closes #9819

@gkalpak gkalpak force-pushed the $route-fix-redirection-with-optional-params branch 3 times, most recently from 496e6ab to a7816d1 Compare November 4, 2014 16:32
@gkalpak gkalpak force-pushed the $route-fix-redirection-with-optional-params branch from a7816d1 to be80b5b Compare November 10, 2014 07:29
@gkalpak
Copy link
Member Author

gkalpak commented Nov 10, 2014

With regard to #9819 (comment), why not merge this one and close the resolved/duplicate issues issues and PRs (as detailed in the aforementioned comment), until #8200 is merge-ready (i.e. has satisfactory test-coverage) ?

@pkozlowski-opensource pkozlowski-opensource self-assigned this Nov 11, 2014
@gkalpak gkalpak force-pushed the $route-fix-redirection-with-optional-params branch from be80b5b to 04f38fd Compare November 11, 2014 10:07
@pkozlowski-opensource
Copy link
Member

The second commit (04f38fd) LGTM, I'm still puzzled about the 018d942. @gkalpak could you help me understand what is going on with 018d942?

I will have another look at this one before merging, putting for 1.3.4 for now.

@gkalpak
Copy link
Member Author

gkalpak commented Nov 11, 2014

@pkozlowski-opensource: The diff for gkalpak@5f54934 is kind of "deceiving". If you take a look at https://github.com/gkalpak/angular.js/blob/22ddfe570aaa2855f24bf3655c757f1d9226209b/test/ngRoute/routeSpec.js#L265 there is this route defined:

$routeProvider.when('/$test.23/foo*(bar)/:baz',

What the tests are trying to show is that the . (in $test.23) and the * (in foo*(bar)) are literally matched (i.e. they are only going to be matched against a . and a * respectively - and not interpreted as special RegExp characters).

The 1st test tests for the . by substituting $test.23 with $testX23 and verifying that the route does not match. Therefore, it should be named matches literal . (not *).
The 2nd test tests for the * by substrituting foo*(bar) with foooo(bar) and verifying that the route does not match. Therefore, it should be named matches literal * (not .).

(Copied from: https://github.com/gkalpak/angular.js/commit/22ddfe570aaa2855f24bf3655c757f1d9226209b#commitcomment-8525121)

@gkalpak gkalpak force-pushed the $route-fix-redirection-with-optional-params branch 3 times, most recently from 641a51d to 0265bea Compare November 14, 2014 20:33
@pkozlowski-opensource
Copy link
Member

@gkalpak OK, I see, this makes sense. Thnx for clarifying.

@pkozlowski-opensource
Copy link
Member

@gkalpak one more question - in your comment you are suggesting that that this PR should be closed in favour of #8200. Is it still true?

@gkalpak
Copy link
Member Author

gkalpak commented Nov 16, 2014

@pkozlowski-opensource: I never said my PR should be closed in favor of #8200 (in those exact words :P).
My suggested "roadmap" is detailed in my other comment you mentioned.

Basically, I would merge this PR (mainly for the test-name fixes and the additonal test-case).
Then I would (rebase and) merge #8200 (as it solves an extra corner-case issue and I find the approach more clear anyway).
Finally, I would close related issues/PRs (see my other comment).

@gkalpak gkalpak force-pushed the $route-fix-redirection-with-optional-params branch 2 times, most recently from 6442ff1 to 0efb43c Compare November 19, 2014 15:10
Previously, when (automatically) redirecting from path that fetured a
trailing slash and optional or "eager" parameters, the resulting path
would (incorrectly) contain the special characters (`?`,`*`) along with
the parameter values.

Closes angular#9819
@gkalpak gkalpak force-pushed the $route-fix-redirection-with-optional-params branch from 0efb43c to b414e5e Compare November 21, 2014 00:47
@googlebot
Copy link

Thanks for your pull request.

It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA) at https://cla.developers.google.com/.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check the information on your CLA or see this help article on setting the email on your git commits.

Once you've done that, please reply here to let us know. If you signed the CLA as a corporation, please let us know the company's name.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ngRoute interpolate route with optional parameters
4 participants