-
-
Notifications
You must be signed in to change notification settings - Fork 109
Fix issue 288 - Lexer fails to detect "*" as a wildcard. #289
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
Conversation
Codecov Report
@@ Coverage Diff @@
## QA #289 +/- ##
=====================================
Coverage 100% 100%
- Complexity 1867 1874 +7
=====================================
Files 63 63
Lines 4530 4540 +10
=====================================
+ Hits 4530 4540 +10 |
|
It looks good I will review on my computer a second time Thank you! |
I'm thinking of different ways to make them a little bit more readable, but maybe we should talk about it in another thread. |
Could we discuss and change the format and then rebase your PRs so we can have a clear view on what changes ? |
|
I would prefer this PR being accepted because it is focused to what it's doing: to fix the lexer issue about the "*" as a wildcard. Review the format of the tests is something that should be part of a separated enhancement IMO. |
|
Okay, I will do the merges soon |
2d88994 to
b962d85
Compare
… flag correctly.
b962d85 to
7095bcc
Compare
|
Thank you @niconoe- |
Fixes #288.
Process is that after the Lexer does his work and created a list of tokens, and before returning back this list, it parses that list in order to change some flags for the
*operator.Doing this by the lexer because it's his job, but it would be too complicated and heavier in performance to directly detect the good flag for this operator. So the job of solving their ambiguity is done later, when the token list is created.
Lots of tests have been updated and 2 new providers came: first to check each case where operator
*is arithmetic (all flags must be1), and second to check each case where operator*is a wildcard (all flags must be16).In those new providers, some tests are not performed beause of a bug related in #287 because of
*/*string found. Once the PR fixing #287 merged into the good branches, one can just uncomment those tests.If you feel to add more tests, please do so or tell me what do you want to test.
Finally, If that way of processing is OK for you guys, I think it can also solve the flag ambiguity of
=(WHERE foo = bar!=@i = 0) using the same process.Thanks a lot, and have a good review!
EDIT: Oh and, this time, @williamdes I did see to start from branch QA, so no cherry-pick to do ;).