Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
Add named capturing groups to regexpToRegexp method #225
Conversation
|
This looks really great to me, thanks! From my understanding this will work in all current projects because the named group doesn't remove it from the array of RegExp results (e.g. this code depends on the position of keys in the result: Line 405 in 98ab888 For getting this feature into the current version of Express, you'll probably need to make this PR against an older branch here: https://github.com/pillarjs/path-to-regexp/blob/0.1.x/index.js However, I believe this it can land in the next Express.js release with the current implementation. |
| @@ -453,13 +453,17 @@ export type Token = string | Key; | |||
| function regexpToRegexp(path: RegExp, keys?: Key[]): RegExp { | |||
| if (!keys) return path; | |||
|
|
|||
| // Use a negative lookahead to match only capturing groups. | |||
| const groups = path.source.match(/\((?!\?)/g); | |||
| const groups = path.source.match(/\((\?<.*?>)?(?!\?)/g); | |||
blakeembrey
Jul 7, 2020
Member
Suggested change
const groups = path.source.match(/\((\?<.*?>)?(?!\?)/g);
const groups = path.source.match(/\((?:\?<(.*?)>)?(?!\?)/g);
Something like this will resolve the comment you have below.
| const groups = path.source.match(/\((\?<.*?>)?(?!\?)/g); | |
| const groups = path.source.match(/\((?:\?<(.*?)>)?(?!\?)/g); |
Something like this will resolve the comment you have below.
DamianKu
Jul 7, 2020
Author
Contributor
Yeah, non-capturing groups, I tried that. Had no luck so I'm guessing, I made some mistake :)
Will try this :)
Yeah, non-capturing groups, I tried that. Had no luck so I'm guessing, I made some mistake :)
Will try this :)
DamianKu
Jul 8, 2020
Author
Contributor
Something like this will resolve the comment you have below.
That didn't work
$ curl http://localhost:1337/testRegex/testData/extraStuff -w "\n"
{"0":"testRegex","2":"extraStuff","(?<groupname>":"testData"}
Something like this will resolve the comment you have below.
That didn't work
$ curl http://localhost:1337/testRegex/testData/extraStuff -w "\n"
{"0":"testRegex","2":"extraStuff","(?<groupname>":"testData"}
blakeembrey
Jul 8, 2020
Member
Oh yeah, that makes sense. It's happening with .match which is only getting the matches and not the groups. We probably need to use regexp.exec in a loop instead.
There's also https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/matchAll but it's not supported on earlier node.js versions or browsers.
Oh yeah, that makes sense. It's happening with .match which is only getting the matches and not the groups. We probably need to use regexp.exec in a loop instead.
There's also https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/matchAll but it's not supported on earlier node.js versions or browsers.
DamianKu
Jul 11, 2020
Author
Contributor
Using exec worked :)
Looping through exec results doesn't look nice tho.
Any idea how to make it nicer?
Using exec worked :)
Looping through exec results doesn't look nice tho.
Any idea how to make it nicer?
|
Test is failing on node v8 as Named capture groups are behind '--harmony' runtime flag.
|
|
@DamianKu You'll probably need to put the tests behind a version flag by checking the node.js version in the test suite. |
Use exec instead of match.
@blakeembrey |
You can read the node.js version here: https://nodejs.org/api/process.html#process_process_version. And use something like https://www.npmjs.com/package/semver to compare whether you want to run it. Put it within an Here's an example (albeit using TypeScript): https://github.com/TypeStrong/ts-node/blob/866bce6a175ec124fe62f269b22e91c92b40f875/src/index.spec.ts#L137 |
|
Hah OK
…On Tue, 14 Jul 2020, 02:16 Blake Embrey, ***@***.***> wrote:
How I would go about doing that?
You can read the node.js version here:
https://nodejs.org/api/process.html#process_process_version. And use
something like https://www.npmjs.com/package/semver to compare whether
you want to run it. Put it within an if condition to define the test.
Here's an example (albeit using TypeScript):
https://github.com/TypeStrong/ts-node/blob/866bce6a175ec124fe62f269b22e91c92b40f875/src/index.spec.ts#L137
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#225 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEE7BAKDB6CZDWQ7RT4OIT3R3OWW5ANCNFSM4OKTOBNA>
.
|
|
I added the If you prefer I can do ternary in const TESTS: Test[] = [
...,
...(gte(process.version, "10.0.0")
? [
[...test...],
[...test...]
]
: ([] as any)),
...,
];
|
| modifier: "", | ||
| pattern: "" | ||
| }); | ||
| index++; |
blakeembrey
Jul 20, 2020
Member
It would probably be good to stay consistent with the behavior in string tokens. In that method, we're only incrementing the counter each non-named group:
Line 189
in
8b0ae94
.
That would mean you probably want execResult[1] || index++. We should add a mixed test of named and unnamed groups to make sure it doesn't break.
It would probably be good to stay consistent with the behavior in string tokens. In that method, we're only incrementing the counter each non-named group:
Line 189 in 8b0ae94
That would mean you probably want execResult[1] || index++. We should add a mixed test of named and unnamed groups to make sure it doesn't break.
DamianKu
Jul 25, 2020
•
Author
Contributor
OK will do that.
Does it mean that this /\/(.+)\/(?<groupname>.+)\/(.+)/ called with /testRegex/testData/extraStuff should result in {"0":"testRegex","1":"extraStuff","groupname":"testData"} ?
OK will do that.
Does it mean that this /\/(.+)\/(?<groupname>.+)\/(.+)/ called with /testRegex/testData/extraStuff should result in {"0":"testRegex","1":"extraStuff","groupname":"testData"} ?
…ith non named regexp matching
|
Thanks @DamianKu! This looks great. |
I was looking into fixing issues raised on express repository #4277.
I modified regular expression in
regexpToRegexpto match not only(but also(?<*>probably regexp could be better :).No change with normal regexp matching,
router.get(/\/regex\/(.+)/)->localhost/regex/testDatawill result in{"0":"testData"}.With named capturing groups,
router.get(/\/regex2\/(?<groupname>.+)/)->localhost/regex2/testDatawill result in{"groupname":"testData"}.Mixed matched with named capturing groups,
router.get(/\/(.+)\/(?<groupname>.+)\/(.+)/)->localhost/testRegex/testData/extraStuff"will result in{"0":"testRegex","2":"extraStuff","groupname":"testData"}.All tests are passing but I didn't add new tests to cover this functionality yet.
Would be great to get some feedback from you guys if I'm even looking in good place or is it worth working on it.