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
DX: Tokens::insertSlices - groom code and fix tests #6172
Conversation
@@ -124,9 +124,7 @@ jobs: | |||
env: | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
==> 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comments, I think all changes were ok
@@ -1387,8 +1387,7 @@ public function testInsertSlicesAtMultiplePlaces(string $expected, array $slices | |||
16 => $slices, | |||
6 => $slices, | |||
]); | |||
|
|||
static::assertSame($expected, $tokens->generateCode()); | |||
static::assertTokens(Tokens::fromCode($expected), $tokens); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the PHP version has value and is not harming anything
Thank you @keradus. |
follows up #6171