DX: Tokens traversing refactoring

[julienfalque]

No description provided.

[julienfalque]

For now those new methods are only here to ease writing type-safe code. However I think they would be a nice addition (replacement?) to current API but I'm not sure about their signature, hence the @internal flag. WDYT?

[keradus]

I'm not sure if I understand the signature difference of input params. what's the change and benefit?

[julienfalque]

The only difference in parameters is that $tokens is not a list of token kinds/prototypes but a single one. I think in most cases we actually need to look for a single kind of token but the current method needs it to be wrapped in an array. It also accepts a single token id. So instead of passing [[T_FOO]] we can now pass T_FOO. By the way, $tokens should be renamed to $token.

I also thought about dropping the $caseSensitive parameter in favor of a distinct method, but I kept it for now.

[keradus]

to be honest I'm not sure if we should add more and more methods into Tokens class. maybe we should aim for dedicated traversable/search classes instead?

[coveralls]

Pull Request Test Coverage Report for Build 3840414199

• 31 of 32 (96.88%) changed or added relevant lines in 4 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.01%) to 92.941%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/Fixer/FunctionNotation/CombineNestedDirnameFixer.php	7	8	87.5%

Totals
Change from base Build 3838999330:	0.01%
Covered Lines:	22579
Relevant Lines:	24294

---

💛 - Coveralls

[keradus]

those changes could go potentially as separated PR, without having all topics concluded, thus those changes could effectively be merged sooner

[keradus]

@julienfalque , I would love to break this PR into smaller parts. Too big change at once.

Let's pick a next/prev method you want to change, and let's see if we can drop usage of old-styled method in our own repo, and if we can, let's merge that one first.
Then we will have the knowledge how it should look, like single/multi-token kind needle thingy.

then we look to further methods

[keradus]

case 1

ref #6680 (comment)

we may or may not be OK to find the previous index. it's handy to assume it's there and crash otherwise. if we are OK that it is not existing, we may need to catch this error.

whatever is the reason, we want to catch the error and handle it in-code, or we bail the app and crash on console, let's use custom exception instead of most-generic one

[julienfalque]

I'm not sure I understand. If one knows the token exists, there already is an API that returns null in such case. Using this new one in a try/catch looks less convenient, it's not its purpose.

If we leave this scenario aside, I don't think a custom exception would be relevant.

[keradus]

I wonder to keep only one method ultimately.

Please consider that we may not know if there will be, or will not be, given token to be find. [eg https://github.com/PHP-CS-Fixer/PHP-CS-Fixer/blob/ff49030e8508ea9b71091b6fdf94de858d898cdc/src/Tokenizer/Analyzer/WhitespacesAnalyzer.php#L27 ]

Also, I would prefer to keep generic exceptions thrown by engine, while narrow, custom exceptions by app logic - better to debug it on unexpected crash.

For those 2 reasons, I would love to ask you to consider dedicated exception. If someone don't want to catch it, nothing will be worse for them. But if one would like to, it will be easier with custom exception.

[keradus]

case 2

from docs, we allow multiple token kinds, from code - only one?
why we have this conditional conversion $tokens = [$tokens]; ?

can we run getPrevTokenOfKind / getNextTokenOfKind analysis to see how many cases we have when we pass single token vs when we pass multiple tokens to match against ?

[julienfalque]

from docs, we allow multiple token kinds, from code - only one?

Actually the doc also indicates only one token is expected. The parameter name is wrong though.

why we have this conditional conversion $tokens = [$tokens]; ?

Because if we look for a token id, the current method expectes it to be wrapped in an array.

can we run getPrevTokenOfKind / getNextTokenOfKind analysis to see how many cases we have when we pass single token vs when we pass multiple tokens to match against ?

I could find more than 200 calls to both methods. Close to 60 of them provide multiple tokens to match against.

[keradus]

from docs, we allow multiple token kinds

Actually the doc also indicates only one token is expected.

in docs, you put $tokens possible tokens, which is plural [in both, description and variable name]

I could find more than 200 calls to both methods. Close to 60 of them provide multiple tokens to match against.

If you would agree with goal to have only one method [that is throwing exception] and not one [that is throwing exception] and other [that is returning null], I believe we should keep possibility to pass multiple allowed token kinds

[keradus]

case 3

not saying to do it right here, right now, but do we aim to deprecate getPrevTokenOfKind etc?
overall, I am getting overwhelmed with amount of traverse methods.
if we aim to deprecate, we should prove these new interfaces in battle and use them in our filters, to see if they really fit needs of the fixers.

[julienfalque]

That's exactly my intention: experiment with new methods and maybe later deprecate some of the current ones.

[keradus]

let's have #6680 (comment) aligned first, then

[keradus]

case 5

I wonder if we should make new, alternative method here, or adjust behaviour of original method.
imagine we deprecate and later remove old method - all fixers will need to start using other method. including the custom fixers.
maybe we can groom behaviour of existing method behind toggle [aka legacyMode] and keep same method in place, trigger deprecation warning when working in legacyMode and exception otherwise?

[keradus]

ref https://github.com/PHP-CS-Fixer/PHP-CS-Fixer/blob/v2.19.3/src/Tokenizer/Tokens.php#L96
potentially with combination of PHP_CS_FIXER_FUTURE_MODE

[julienfalque]

I prefer multiple specific functions over a single one with flag parameters because it's more readable and often more type-safe. But the main reason for me to add a new method was that the name of the current method has always been a bit confusing to me: when reading get…Token(), I think it will return Token|null while it actually returns int|null. I wanted to prevent that with the new name.

[keradus]

Imho:

we have method_A now, that works in given way.

We should align how we would like to have it working, with a way that we can easily migrate codebase [eg migration from null-return to exception-raised is relatively easy, as we don't expect missing token in majority of places ; yet migration from multi-kind needle to single-kind needle will be a massing change to apply]. Then, we should make the method trigger warnings [not exception] if someone would use it in a way that will work differently in next version of method. And then, we could have compact toggle that is raising the warning or actually changing the logic [eg raising exception instead of returning null]. I believe as long as we don't change the single/multi-kind needle, we do not need to change prototype of the method.

We can decide to introduce temporary method of method_Av4 that would be a proxy to method_A with forcing future-mode [eg throwing exception and not warning]. but open idea here, only.

I'm not sure about renaming the methods for renaming itself - we got such name for a quite long time, and changing the name is not that big benefit, as ppl used to them.