Skip to content

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

Merged
merged 1 commit into from
Mar 20, 2020

Conversation

niconoe-
Copy link
Contributor

@niconoe- niconoe- commented Feb 3, 2020

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 be 1), and second to check each case where operator * is a wildcard (all flags must be 16).
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 ;).

@codecov
Copy link

codecov bot commented Feb 3, 2020

Codecov Report

Merging #289 into QA will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##               QA   #289   +/-   ##
=====================================
  Coverage     100%   100%           
- Complexity   1867   1874    +7     
=====================================
  Files          63     63           
  Lines        4530   4540   +10     
=====================================
+ Hits         4530   4540   +10

@williamdes
Copy link
Member

It looks good
But our storing format is not readable on PRs, something needs to be done some day

I will review on my computer a second time

Thank you!

@niconoe-
Copy link
Contributor Author

niconoe- commented Feb 3, 2020

It looks good
But our storing format is not readable on PRs, something needs to be done some day

I'm thinking of different ways to make them a little bit more readable, but maybe we should talk about it in another thread.

@williamdes
Copy link
Member

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 ?
Or do you prefer that I merge your work but in that case we will not have another occasion to try in live the new diff

@williamdes williamdes changed the title Fix issue 288 Fix issue 288 - Lexer fails to detect "*" as a wildcard. Feb 9, 2020
@niconoe-
Copy link
Contributor Author

niconoe- commented Feb 9, 2020

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.

@williamdes
Copy link
Member

Okay, I will do the merges soon
🇫🇷

@williamdes williamdes merged commit ae228e9 into phpmyadmin:QA Mar 20, 2020
@williamdes williamdes self-assigned this Mar 20, 2020
@williamdes williamdes added this to the 4.5.1 milestone Mar 20, 2020
@williamdes
Copy link
Member

Thank you @niconoe-
Merged !

@niconoe- niconoe- deleted the fix-issue-288 branch February 27, 2023 14:51
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.

2 participants