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 unicodeSet option for the RegExp set notation proposal #113

Merged
merged 2 commits into from Sep 6, 2021

Conversation

nicolo-ribaudo
Copy link
Collaborator

@nicolo-ribaudo nicolo-ribaudo commented Aug 16, 2021

https://github.com/tc39/proposal-regexp-set-notation

I implemented support for the proposed v flag, which can only be used when the unicodeSet: true option is enabled. I updated the AST in a bakward-compatible way: characterClass nodes now have a kind property that can be "union" (default), "intersection" or "subtraction".

If you have ideas for new tests please let me know!

After that this PR is merged, I will add support in regjsgen and regexpu-core.

cc @mathiasbynens @bnjmnt4n @jviereck (no hurry for reviews, since starting from Wednesday I'm on vacation for two weeks)

@jviereck
Copy link
Owner

Thanks for your PR! I am on holidays this week and will look at this next week.

@jviereck
Copy link
Owner

jviereck commented Sep 4, 2021

This looks very good. I took a closer look at the PR and there were zero review comments needed (!). Well done @nicolo-ribaudo .

@nicolo-ribaudo do you have any second thoughts in the meantime about this PR or do you want me to merge and create a new release?

@nicolo-ribaudo
Copy link
Collaborator Author

Awesome, thanks!

During the last TC39 meeting (last week) there has been some discussion about this proposal. From what I understood it was only about semantics and not about the syntax, but I'd like to wait for @mathiasbynens to confirm it.

@mathiasbynens
Copy link
Collaborator

Awesome, thanks!

During the last TC39 meeting (last week) there has been some discussion about this proposal. From what I understood it was only about semantics and not about the syntax, but I'd like to wait for @mathiasbynens to confirm it.

Confirmed. The main syntax hasn't changed. It might of course still change on the way to Stage 3 (as with all proposals) but high-level changes are unlikely at this point. I would love to see this land!

@nicolo-ribaudo
Copy link
Collaborator Author

nicolo-ribaudo commented Sep 4, 2021

This PR considers the u and v flags as independent. Is it still correct?

If the answer is yes, then this is good to land and I'll update the regjsgen PR 👍

@mathiasbynens
Copy link
Collaborator

mathiasbynens commented Sep 4, 2021

This PR considers the u and v flags as independent. Is it still correct?

They are independent, indeed. The only "gotcha" is that they cannot be combined, i.e. /./uv throws. Background: tc39/proposal-regexp-v-flag#23 (comment)

@nicolo-ribaudo
Copy link
Collaborator Author

Ok, I have to add that error 👍

@nicolo-ribaudo
Copy link
Collaborator Author

@jviereck If the last commit looks good this can be merged.

@jviereck jviereck merged commit 86b5907 into jviereck:gh-pages Sep 6, 2021
@jviereck
Copy link
Owner

jviereck commented Sep 6, 2021

I released v0.7.0 including this PR, which was published to npm.

@nicolo-ribaudo nicolo-ribaudo deleted the proposal-unicode-set branch September 6, 2021 17:23
bnjmnt4n pushed a commit to bnjmnt4n/regjsgen that referenced this pull request Sep 29, 2021
* Add support for the RegExp set notation proposal

Proposal: https://github.com/tc39/proposal-regexp-set-notation
RegJSParser PR: jviereck/regjsparser#113

* Use `regjsparser` from npm

* `u` and `v` cannot be used together

* Avoid default value for `separator`
@nicolo-ribaudo
Copy link
Collaborator Author

@jviereck I didn't notice that the proposal was updated, and now uses \q{ab|cd} for string alternatives rather than (ab|cd). I can open a PR to update it tomorrow (or probably on Tuesday), if you want to wait to release the next version until this is fixed.

@jviereck
Copy link
Owner

jviereck commented Dec 6, 2021

Thanks for the heads-up @nicolo-ribaudo . I will wait for your change before drafting a new release then.

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

Successfully merging this pull request may close these issues.

None yet

3 participants