Add support for named capturing groups #83
Conversation
|
Hi @nicolo-ribaudo, thanks a lot for implementing this and proving the PR - especially as you went with using a different parser in the end :) Doing a quick look over the code, is the named-group feature enabled by default? When adding unicode parsing before (see 1), we've added a If you have any questions, let me know. Thanks! \jv |
|
I enabled named groups by default because the only thing missing for the proposal to be merged to the main spec is the final approval, so it would be enabled by default[1] really soon (relevant meeting notes, tc39/ecma262#1027). If you still prefer to enable this feature using a flag, let me know 😉 [1] What I say is only valid if the |
Yes, I prefer to have this feature enabled via a flag similar as in 1. The reasoning is to avoid people using the library to undergo possible breaking/functionality change when adding this feature. Thanks. If there is anything, let met know. \jv |
|
Done! |
|
hi folks, First: Thanks @mathiasbynens for picking this one up again and asking me for review. I dropped the todo here. This is not the way it should be. Sorry @nicolo-ribaudo for delaying the review by this. Overall: Thanks @nicolo-ribaudo for providing this PR. The code looks very clean and I like how you managed to follow the coding style of the library. There are a few small nits, that I am going to comment code-inline next. If you have any questions, please let me know :), All the best & thanks, |
There was a problem hiding this comment.
There are a few small nits, that I am going to comment code-inline next.
@nicolo-ribaudo: Please let me know if you want to do the follow-up work yourself. Feel free to say no and I can take care of it ;)
Done :)
| @@ -58,6 +58,7 @@ | |||
| // DecimalEscape | |||
| // CharacterEscape | |||
| // CharacterClassEscape | |||
| // k GroupName | |||
There was a problem hiding this comment.
For understanding: the k is matching the character k here?
| // _ | ||
| // \ RegExpUnicodeEscapeSequence | ||
| // <ZWNJ> | ||
| // <ZWJ> |
There was a problem hiding this comment.
nit: shall we move the grammar for this up at the beginning of the file?
There was a problem hiding this comment.
Looking over the code once more, other parts of the grammar are also defined locally. Therefore, leaving the grammar definition at the place introduced in this PR.
| @@ -0,0 +1,60 @@ | |||
| { | |||
There was a problem hiding this comment.
Out of curiosity: Where did you/@nicolo-ribaudo get these tests from? Are these the official ECMA-262 tests for the named-groups feature?
There was a problem hiding this comment.
No, I wrote them by myself.
| }); | ||
| runTests(require('./test-data-named-groups-unicode.json'), 'u', { | ||
| namedGroups: true | ||
| }); |
There was a problem hiding this comment.
How about adding tests for named groups & unicode-properties enabled jointly?
| console.log( | ||
| '// ECMAScript 5.1/Unicode v%s NonAsciiIdentifierPartOnly:\n\n%s', | ||
| version, | ||
| result.NonAsciiIdentifierPartOnly |
There was a problem hiding this comment.
Ah, strange to console.log only one of the parts. Good catch :)
There was a problem hiding this comment.
Well, the other part wasn't used yet 🙂
|
Do you mind if I wait with releasing a new version of the library once the jointly tests for the features are there? |
https://github.com/tc39/proposal-regexp-named-groups
I started working on this feature because I needed it for implementing a babel plugin. I then decided to use another package which already transformsnamed groups in regexps (regexp-tree), but I finished this PR anyway since it will probably be useful to someone else.