-
-
Notifications
You must be signed in to change notification settings - Fork 115
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
chore(stdlib)!: Add Types Aliases to regex lib #1036
chore(stdlib)!: Add Types Aliases to regex lib #1036
Conversation
Alright Requested Changes Have Been Made |
@spotandjake The suggestion was not only to change those 2 specifically. Those variants contain comments after each that describe what the random |
Ah think I fixed my mistake there |
I also want to get @peblair's eyes on this since he opened the issue and wrote rege |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@spotandjake Thanks for this PR! I spoke about the above conversation with the team, and I think we want to pare back down some of the suggested type aliases. If you could revert the following, then I think this is in a place to get merged:
- 001eeac
- 3bac01f (except we do want to retain the
@since next
changes you made in this commit) - 6788da6
I'm pretty sure that'd cover everything. With that, I'm happy to give this a final scan and give this a final approval.
@peblair feel free to review it now i reverted those changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
@@ -387,7 +394,7 @@ enum ParsedRegularExpression { | |||
RELookbehind(ParsedRegularExpression, Bool, Box<Number>, Box<Number>, Number, Number), // regex, is-match, lb-min, lb-max, n-start, num-n (lb-xx values patched in later) | |||
RECut(ParsedRegularExpression, Number, Number, Bool), // regex, n-start, num-n, needs-backtrack | |||
REReference(Number, Bool), // n, case-sensitive | |||
RERange(List<(Number, Number)>), | |||
RERange(RERange), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is RERange(RERange)
used? Do consumers need to have access to the list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah, they don't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving, assuming the answer to my question is "no, consumers don't need access to it"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again @spotandjake!
closes: #692 .