DX: Tokens::insertSlices - groom code and fix tests#6172
DX: Tokens::insertSlices - groom code and fix tests#6172keradus wants to merge 1 commit intoPHP-CS-Fixer:masterfrom
Conversation
| PHP_CS_FIXER_IGNORE_ENV: ${{ matrix.PHP_CS_FIXER_IGNORE_ENV }} | ||
| FAST_LINT_TEST_CASES: ${{ matrix.FAST_LINT_TEST_CASES }} | ||
| run: | | ||
| php -v |
There was a problem hiding this comment.
this is very much of value as test pass locally but not on gitlab, knowing the PHP version including patch
There was a problem hiding this comment.
I don't think we should add php -v before each PHP command. and if so, putting it in one place is enough.
And we have it https://github.com/FriendsOfPHP/PHP-CS-Fixer/runs/4521649824?check_suite_focus=true#step:4:15
There was a problem hiding this comment.
It is not for each PHP command is it? only for unique phpunit runs.
If you see no value in having the build details than sure remove it.
It will likely be part of each draft PR in the future that want to do fixes for upcoming PHP version so the author knows the build details of nightly builds etc.
There was a problem hiding this comment.
@SpacePossum , all commands within single build are using the same PHP installation. so yes, you can scroll to top of log of given build to see exact PHP version
There was a problem hiding this comment.
I don't understand;
https://github.com/FriendsOfPHP/PHP-CS-Fixer/runs/4516979071?check_suite_focus=true
shows the PHP version PHPUnit is going to use one time,
than here;
https://github.com/FriendsOfPHP/PHP-CS-Fixer/runs/4516979363?check_suite_focus=true
shows the PHP version PHPUnit is going to use one time,
how is this before each PHP command ?
There was a problem hiding this comment.
==> Setup PHP
✓ PHP Found PHP 8.1.0
vs.
PHP 8.1.0 (cli) (built: Nov 28 2021 04:13:56) (NTS)
it is not the same, the second shows more, which is especially handy when running on nightly build.
There was a problem hiding this comment.
each job starts with Setup PHP, then each command within this job is using it.
For example you showed, I don't see anything different. For nightly build, I would love to see example.
If adding php -v at all, I believe we should not put it inside PHPUnit step, but make a dedicated step like PHP info after Setup PHP step.
SpacePossum
left a comment
SpacePossum
left a comment
There was a problem hiding this comment.
see comments, I think all changes were ok
keradus
changed the title
| ]); | ||
|
|
||
| static::assertSame($expected, $tokens->generateCode()); | ||
| static::assertTokens(Tokens::fromCode($expected), $tokens); |
There was a problem hiding this comment.
SpacePossum
added
RTM
SpacePossum
left a comment
SpacePossum
left a comment
There was a problem hiding this comment.
I think the PHP version has value and is not harming anything
|
Thank you @keradus. |
3 participants
follows up #6171