-
Notifications
You must be signed in to change notification settings - Fork 65
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
Fix bug: Replace * with *? in QuantifierToken regex pattern #181
Conversation
@christopher-hampson thank you for your contribution! Do you happen to have any examples where the old code had issues fixed by the new code? It would be nice to have a test case for the behavior this change is specifically addressing. |
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.
@christopher-hampson Yes, I would agree with @eliotwrobson that an additional test case to verify these changes would be preferable, if you could please provide one. This would demonstrate, in a TDD fashion, that the hypothetical omission of these lexer changes would break this new test.
Thanks @eliotwrobson @caleb531, I've added a couple additional tests to test_quantifier which seemed the most appropriate place for this but apologies if not. |
@christopher-hampson it looks like the tests are failing |
Greedy matching for the QuantifierToken appears to cause issues for patterns with more than one quantifier.