Use generators #43
Use generators #43
Conversation
greg0ire
force-pushed
the
use-generators
branch
2 times, most recently
from
bda68e6 to
63dbfd4
Compare
|
Actually I'm going to remove the second commit (for now): I'm getting really weird behavior, and I think it might have to do with the caching that takes place in the |
greg0ire
force-pushed
the
use-generators
branch
2 times, most recently
from
906ae71 to
4030a11
Compare
|
The cache is removed with #44 , so I'm pushing the commit that introduces awareness of previous and next tokens again |
greg0ire
force-pushed
the
use-generators
branch
3 times, most recently
from
10508df to
c2262a6
Compare
src/Token.php
Outdated
| { | ||
| $this->type = $type; | ||
| $this->value = $value; | ||
| if ($previous !== null) { | ||
| $previous->next = $this; |
There was a problem hiding this comment.
This means that a Token is really valid only if there is no token after it, or if you just finished building that token. Combined with generators, this produces bugs, and that's why I couldn't use one for getFilteredTokens… this might be a bad idea. Right now the program works because calls are made carefully, but this is flawed I'm afraid. I think I am going to have to rethink all of this.
There was a problem hiding this comment.
I think I fixed it now, with exceptions that prevent rewiring or accessing next - related features until they are available.
There was a problem hiding this comment.
Keeping as a draft until #45 is merged
greg0ire
force-pushed
the
use-generators
branch
2 times, most recently
from
c639284 to
05128b3
Compare
There was a problem hiding this comment.
Code wise the pull request looks good.
However getFilteredTokens makes in vain the use of generators on the Tokenizer as all the tokens end up in an array.
Another note is how complex is the token object. It has the knowledge of neighbors an more. I understand that the library needs that knowledge, but maybe this is the wrong solution for the problem.
In Masterminds/html5-php, another library I maintain, the Tokenizer https://github.com/Masterminds/html5-php/blob/master/src/HTML5/Parser/Tokenizer.php does not give you a way to get all the tokens, but it offers some methods to get the current, look around and move the cursor. With that method, all the tokenization work is done there.
The consumer class has no knowledge on how the tokens are stored internally.
I think that is a much cleaner approach. (of course it requires a bit more Refactoring on this lib to implement something similar)
|
I spent a crazy amount of time last week-end on this, and it was quite satisfying and fun, but I have the same feeling as you do: the usage of generators is in vain, it brings some clarity and satisfaction in the Tokenizer, but at the price of some danger, that I tried to mitigate with the exceptions in the latest version. I think you are right, and I'm going to have a look at that other library. I'm going to drop the 2 first commits of this PR (I think we can all agree the last one is ok). |
greg0ire
force-pushed
the
use-generators
branch
from
05128b3 to
8d80a27
Compare
|
Not sure why Psalm fails, I think there is a bug with the stub it has for |
greg0ire
force-pushed
the
use-generators
branch
from
048c624 to
8d80a27
Compare
I just got bit by a bug on master at doctrine/sql-formatter#43 (comment), and there are good chances we have the bug on the Dbal too. Anyway, it only makes sense to use the same version as in the vendors.
I just got bitten by a bug on master at doctrine/sql-formatter#43 (comment), and there are good chances we have the bug on the Dbal too. Anyway, it only makes sense to use the same version as in the vendors.
My goal with the second commit was to use generators for
getFilteredTokens, and then I realized it would not be possible: there are lookaheads up to 250 tokens ahead to decide whether to stay inline or to add linebreaks. It made me move a lot of logic to the value objects though, and get rid of the$indexedTokensugly array.