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: add support v flag #82

Merged
merged 16 commits into from
Jul 22, 2023
Merged

feat: add support v flag #82

merged 16 commits into from
Jul 22, 2023

Conversation

ota-meshi
Copy link
Member

@ota-meshi ota-meshi commented Apr 26, 2023

Overview

This PR adds support for v flag.

Add new AST nodes

  • ExpressionCharacterClass
    If the character class has intersection or subtraction it will be wrapped in this node. E.g. [a--b], [a&&b],[^a--b], [^a&&b]
  • ClassIntersection
    It is an intersection node. E.g. a&&b
  • ClassSubtraction
    It is a subtraction node. E.g. a--b
  • ClassStringDisjunction
    It is \p{...} node. E.g. \q{a|b}
  • StringAlternative
    It is Alternative like node in ClassStringDisjunction.

Change AST nodes

  • CharacterClass
    • Add unicodeSets flag. true if the regex has the v flag. It had this because negation (^) behaves differently at runtime.
  • CharacterSet (with kind: "property")
    • Add strings flag. true if it \p{} with property of string (e.g. Basic_Emoji). It has no negation.

Change API

  • The fourth argument of parser.parsePattern and validator.validatePattern has been changed from a boolean uFlag to an object with flags.
    This is because the object format is easier to use than specifying multiple flags in argument order. For backward compatibility, the old boolean value work as well.

close #80

@ota-meshi ota-meshi force-pushed the v-flag branch 3 times, most recently from 4a0e3dc to 9dcfdfe Compare April 27, 2023 00:04
@RunDevelopment
Copy link

Thank you @ota-meshi! I'll review this soon.


There is one thing I noticed by looking at the AST format: Alternative. I think that it's very confusing that Alternative is now either a DisjunctionAlternative or a StringAlternative. The 2 types of alternatives seem very different to me, so I think that combining them under Alternative doesn't make much sense.

I also think that the new Alternative just isn't a useful type. In the entire AST definition, the Alterantive type is used 2 times, while DisjunctionAlternative is used 20 times and StringAlternative used 2 times. DisjunctionAlternative and StringAlternative aren't used together much.

So I think it would make sense to more clearly separate the new StringAlternative. I would keep the old Alternative definition and make a new type for StringAlternative.

@ota-meshi
Copy link
Member Author

Good point. I think your idea makes sense. I will change the AST.

@ota-meshi
Copy link
Member Author

ota-meshi commented May 23, 2023

I refactored this PR. Previously, a string indicating the kind was returned from some consume functions and processed. However, this was inconsistent so I refactored the consume function to return a boolean.

8d46d14

@ota-meshi
Copy link
Member Author

I refactored this PR. Changed MayContainStrings to handle using result. This follows a review comment in PR in Acorn.

ffcaed5

@ota-meshi
Copy link
Member Author

@eslint-community/core-team @RunDevelopment
Could you please check this PR if you are interested?
If no one responds, I'll merge and release this PR after next week.

Copy link

@aladdin-add aladdin-add left a comment

Choose a reason for hiding this comment

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

Not very familiar with this repo, but the test cases look good! :)

@ota-meshi
Copy link
Member Author

Thank you for checking out this PR. I will merge this PR.

@ota-meshi ota-meshi merged commit 31dac9d into main Jul 22, 2023
@ota-meshi ota-meshi deleted the v-flag branch July 22, 2023 04:08
@github-actions
Copy link

🎉 This PR is included in version 4.6.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@ota-meshi
Copy link
Member Author

Hi @RunDevelopment!
I invited you to the team. I would be happy if you accept it.

@RunDevelopment
Copy link

Joined. Thanks @ota-meshi!

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

Successfully merging this pull request may close these issues.

Support for v flag and properties of strings
3 participants