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

Add new method TokensList::getPreviousOfType + manage list of types in TokensList::getPreviousOfType and TokensList::getNextOfType #429

Conversation

Tithugues
Copy link
Contributor

@Tithugues Tithugues commented Mar 8, 2023

These changes aims to:

  • support new request for a previous token based on a set of types
  • support request for a next token based on a set of types, not only one

Changes were done in a backward compatible way.

I intentionally didn't change all the getNextOfType* methods, as other methods have an additional filter which does not always make sense to combine with an array of types.

Additionally, this PR relies on changes done in PR #428.

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.

Hi @Tithugues
Are this changes used in some other PR on in of your implementations ?

@niconoe-
Copy link
Contributor

niconoe- commented Mar 9, 2023

I'm working with @Tithugues IRL and yes, we are expecting to use this (especially the getPreviousOfType).

Without entering into business details, we are working on SQL files that can define metadata on SQL statements, basically in the same state of art than PHP Attributes. So, from a statement, we need to parse the bound metadata written in a bash comment (similar to PHP Attribute, and complient with MySQL/MariaDB as such metadata can't be understood by such DBMS) which is written before the statement declaration, so we need to go back. As getPrevious is ignoring comments and there's no other proper way (--$list->idx is not really powerfull and user-friendly) to step back, we got the feeling this feature was lacking.

But not only for our needs, we both are confident that such feature could really help into parsing some complex options or statements into sql-parser in the future, or simplify some parsing methods that are juggling with $list->idx.

@williamdes williamdes self-assigned this Mar 9, 2023
@williamdes williamdes added this to the 5.8.0 milestone Mar 9, 2023
@williamdes
Copy link
Member

I'm working with @Tithugues IRL and yes, we are expecting to use this (especially the getPreviousOfType).

That's awesome to some level !!!
Is @Tithugues French too ?
So cool !
We should probably meet some day in France

Without entering into business details, we are working on SQL files that can define metadata on SQL statements, basically in the same state of art than PHP Attributes. So, from a statement, we need to parse the bound metadata written in a bash comment (similar to PHP Attribute, and complient with MySQL/MariaDB as such metadata can't be understood by such DBMS) which is written before the statement declaration, so we need to go back. As getPrevious is ignoring comments and there's no other proper way (--$list->idx is not really powerfull and user-friendly) to step back, we got the feeling this feature was lacking.

But not only for our needs, we both are confident that such feature could really help into parsing some complex options or statements into sql-parser in the future, or simplify some parsing methods that are juggling with $list->idx.

Very nice, be sure to upstream as much possible, we would love to benefit from your feedback and implementations

@niconoe-
Copy link
Contributor

niconoe- commented Mar 10, 2023

That's awesome to some level !!! Is @Tithugues French too ? So cool ! We should probably meet some day in France

Yes, we are 🙂
It would be a pleasure! Do you agree to stay in touch by mail? (I think I have yours)

Very nice, be sure to upstream as much possible, we would love to benefit from your feedback and implementations.

I'll discuss with @Tithugues tomorrow to see if we could share an example of the benefits this PR could bring.

@williamdes
Copy link
Member

Yes, we are slightly_smiling_face
It would be a pleasure! Do you agree to stay in touch by mail? (I think I have yours)

So cool !, for sure you can mail me at williamdes [at] wdes dot fr
Let's do a meeting, I can travel a bit

@williamdes williamdes merged commit b5c995a into phpmyadmin:5.8.x Mar 11, 2023
@Tithugues Tithugues deleted the feature/list-of-types-for-getpreviousoftype-and-getnextoftype branch March 11, 2023 15:32
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.

3 participants