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

feat: support modifiers proposal #127

Merged
merged 4 commits into from
Jun 26, 2022
Merged

Conversation

liuxingbaoyu
Copy link
Contributor

parser.js Outdated
skip(":");
modifiersGroup.body = parseDisjunction();
if (!modifiersGroup.body) {
bail('Expected disjunction');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a test case for this branch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't find such an example.😰

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does /(?i:)/ or /(?i:|)/ throw?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no

@jviereck
Copy link
Owner

Thanks for all the work on this one. I plan to take a look over the weekend.

@JLHwung
Copy link
Collaborator

JLHwung commented Jun 16, 2022

Can you also update

export type Features = {
?

@tjenkinson
Copy link
Contributor

There’s some tests for types too :)

@jviereck
Copy link
Owner

This looks overall good to me. Let's figure out the latest discussion about "We should parse [...]" and then this should be good to go.

Copy link
Collaborator

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

After re-reviewing, I now think that the PR is fine as-is. If there are no modifiers, we don't need to add an empty object to the current (?:...) AST.

@jviereck jviereck merged commit 524fbd5 into jviereck:gh-pages Jun 26, 2022
@jviereck
Copy link
Owner

Thank you all for working on this one.

I've created a new npm release 0.9.0 of the library including this change.

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

Successfully merging this pull request may close these issues.

6 participants