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

Cursor based tokenizer #48

Merged
merged 3 commits into from
Apr 21, 2020
Merged

Cursor based tokenizer #48

merged 3 commits into from
Apr 21, 2020

Conversation

goetas
Copy link
Member

@goetas goetas commented Apr 19, 2020

No description provided.

@goetas goetas marked this pull request as draft April 19, 2020 07:22
@goetas goetas force-pushed the tokenizer branch 3 times, most recently from 2f95fc3 to da33754 Compare April 20, 2020 07:06
@goetas goetas marked this pull request as ready for review April 20, 2020 07:06
@goetas goetas requested a review from greg0ire April 20, 2020 07:09
Copy link
Member

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

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

I absolutely love what you did here, it's beautiful 👌

src/Cursor.php Outdated Show resolved Hide resolved
src/Cursor.php Outdated Show resolved Hide resolved
src/Cursor.php Outdated Show resolved Hide resolved
src/SqlFormatter.php Outdated Show resolved Hide resolved
@greg0ire greg0ire added this to the 1.0.0 milestone Apr 20, 2020
if (isset($originalTokens[$indexedToken['originalIndex']-1]) &&
! $originalTokens[$indexedToken['originalIndex']-1]->isOfType(Token::TOKEN_TYPE_WHITESPACE)) {
$prevToken = $cursor->subCursor()->previous();
if ($prevToken && ! $prevToken->isOfType(Token::TOKEN_TYPE_WHITESPACE)) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not requiring you do it, but how would you feel about implementing the null object pattern to avoid all these checks?

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm. not sure about that. to do so i will have to introduce a TokenInterface and also to use an instanceof check in the while, or add one more method in the Cursor class... do you have a suggestion on how to implement it?

Copy link
Member

Choose a reason for hiding this comment

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

I would rather add a new type, and check it in most methods if needed (but isOfType would not need to change, for instance). value() would require throwing, so would withValue

Copy link
Member

Choose a reason for hiding this comment

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

Ah drop it, this is probably just another bad design idea on my part 😓

@goetas
Copy link
Member Author

goetas commented Apr 21, 2020

I have marked the Tokenizer as internal, do not see why someone else should use this class.

@goetas goetas requested a review from greg0ire April 21, 2020 06:33
greg0ire
greg0ire previously approved these changes Apr 21, 2020
@greg0ire
Copy link
Member

Please squash some of the commits together (for instance the one that removes the debugging variable)

goetas and others added 3 commits April 21, 2020 13:37
@greg0ire greg0ire merged commit 692d0cd into master Apr 21, 2020
@greg0ire greg0ire deleted the tokenizer branch April 21, 2020 12:16
@greg0ire greg0ire restored the tokenizer branch April 21, 2020 12:16
@greg0ire greg0ire deleted the tokenizer branch April 21, 2020 12:17
@greg0ire
Copy link
Member

Thanks @goetas !

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