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

For Contexts, rely on token constants + Add MySQL 8.4 context #569

Conversation

niconoe-
Copy link
Contributor

@niconoe- niconoe- commented Aug 6, 2024

EDIT: I'll probably close this one in profit of #570

I'm taking this from @Tithugues, enhancing the generator itself rather than doing the job "manually".

Main changes are:

  • Stop using magic numbers to use const instead in both generators and generated contexts
  • As generated contexts are now formatted in a way each word has its own line, simplify the builder
    • Words are still sorted by types, then by alphanumerical order, but no longer by length, as it no longer make sense
    • Some comments/phpDoc are fixed on the generated contexts
  • Fix reserved keywords from MariaDB 10.3, based on the list given at https://mariadb.com/kb/en/reserved-words/
  • Add MySQL 8.4 context, using changes described on https://dev.mysql.com/doc/refman/8.4/en/keywords.html to make the list.

Please let me know if something is wrong or is missing 😉

Copy link
Contributor

@kamil-tekiela kamil-tekiela left a comment

Choose a reason for hiding this comment

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

I like it. I wanted to propose something similar myself.

Copy link
Member

@williamdes williamdes left a comment

Choose a reason for hiding this comment

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

I would say we want the changes for target branch 5.10.0 ?

@niconoe-
Copy link
Contributor Author

niconoe- commented Aug 6, 2024

I would say we want the changes for target branch 5.10.0 ?

I don't, know TBH. I targeted master just as @Tithugues did on #568 , but if you want this on another tag, I'll adapt.

@williamdes
Copy link
Member

I would say we want the changes for target branch 5.10.0 ?

I don't, know TBH. I targeted master just as @Tithugues did on #568 , but if you want this on another tag, I'll adapt.

Well, that's okay for me both ways
But the changes will not be released very soon but only when the major version will be out

@niconoe-
Copy link
Contributor Author

niconoe- commented Aug 6, 2024

I would say we want the changes for target branch 5.10.0 ?

I don't, know TBH. I targeted master just as @Tithugues did on #568 , but if you want this on another tag, I'll adapt.

Well, that's okay for me both ways But the changes will not be released very soon but only when the major version will be out

To me, that's not a BC Break change at all, so it shouldn't be requested for the next major.
And to be honest, beside the addition of the MySQL 8.4 context, there's nothing new too, so we might even consider this as a patch 😆

What's the target branch if I want to propose it for the next 5.9 ?

@williamdes
Copy link
Member

What's the target branch if I want to propose it for the next 5.9 ?

https://github.com/phpmyadmin/sql-parser/tree/5.9.x

That will be merged up to master

@niconoe- niconoe- changed the base branch from master to 5.9.x August 6, 2024 12:51
@niconoe- niconoe- changed the base branch from 5.9.x to master August 6, 2024 12:51
@niconoe-
Copy link
Contributor Author

niconoe- commented Aug 6, 2024

Well, I didn't realized that I wrote in PHP 8.2 syntax, but 5.9.x is still on PHP 7.2 minimum, so it won't be OK.

I'll need to adapt the code 😢

@niconoe-
Copy link
Contributor Author

Closing this one in profit of #570

@niconoe- niconoe- closed this Aug 13, 2024
@niconoe- niconoe- deleted the feature/define-context-keywords-relying-on-token-constants branch August 13, 2024 09:59
@williamdes williamdes self-assigned this Aug 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants