Move phpcs from Travis to GitHub Actions #3979
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,28 +6,66 @@ on: | |
|
|
||
| jobs: | ||
| static-analysis-psalm: | ||
| name: "Static Analysis with Psalm" | ||
| runs-on: "ubuntu-latest" | ||
|
|
||
| strategy: | ||
| matrix: | ||
| php-version: | ||
| - "7.4" | ||
|
|
||
| steps: | ||
| - name: "Checkout code" | ||
| uses: "actions/checkout@v2" | ||
|
|
||
| - name: "Install PHP" | ||
| uses: "shivammathur/setup-php@v2" | ||
| with: | ||
| coverage: "none" | ||
| php-version: "${{ matrix.php-version }}" | ||
|
|
||
| - name: "Cache dependencies installed with composer" | ||
| uses: "actions/cache@v1" | ||
| with: | ||
| path: "~/.composer/cache" | ||
| key: "php-${{ matrix.php-version }}-composer-locked-${{ hashFiles('composer.lock') }}" | ||
|
There was a problem hiding this comment. This looks like an unnecessary complication. What's the scenario where we're going to run the code style check using different PHP versions? There was a problem hiding this comment. This has already been discussed. It is trying to maintain consistency across repos There was a problem hiding this comment. Could you point me to the thread, please? I could find the discussion regarding the build matrix. There was a problem hiding this comment. There was a problem hiding this comment. sorry, let me elaborate, because that link doesn't help you. I think, even if the matrix has a single php version, it enables us to use the same key: "php-${{ matrix.php-version }}-composer-locked-${{ hashFiles('composer.lock') }}"where a matrix is used and key: "php-7.4-composer-locked-${{ hashFiles('composer.lock') }}"where a matrix is not used. There was a problem hiding this comment. So, by “everywhere” you mean that if named consistently, the same cache entry can be shared between multiple actions? Makes sense in this case. There was a problem hiding this comment. Multiple action in the same repo yes. Thanks for phrasing it better! |
||
| restore-keys: "php-${{ matrix.php-version }}-composer-locked-" | ||
|
|
||
| - name: "Install dependencies with composer" | ||
| run: "composer install --no-interaction --no-progress --no-suggest" | ||
bendavies marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| - name: "Psalm" | ||
| run: "vendor/bin/psalm" | ||
|
|
||
| coding-standards: | ||
| name: "Coding Standards" | ||
| runs-on: "ubuntu-latest" | ||
|
|
||
| strategy: | ||
| matrix: | ||
|
There was a problem hiding this comment. I believe the whole point of static analysis is that it's static and doesn't depend on the runtime environment. Having a matrix here looks like overkill to me. There was a problem hiding this comment. It is trying to maintain consistency across repos |
||
| php-version: | ||
| - "7.4" | ||
|
|
||
| steps: | ||
| - name: "Checkout" | ||
| uses: "actions/checkout@v2" | ||
|
|
||
| - name: "Install PHP" | ||
| uses: "shivammathur/setup-php@v2" | ||
| with: | ||
| coverage: "none" | ||
| php-version: "${{ matrix.php-version }}" | ||
| tools: "cs2pr" | ||
|
|
||
| - name: "Cache dependencies installed with composer" | ||
| uses: "actions/cache@v1" | ||
| with: | ||
| path: "~/.composer/cache" | ||
| key: "php-${{ matrix.php-version }}-composer-locked-${{ hashFiles('composer.lock') }}" | ||
| restore-keys: "php-${{ matrix.php-version }}-composer-locked-" | ||
|
|
||
| - name: "Install dependencies with composer" | ||
| run: "composer install --no-interaction --no-progress --no-suggest" | ||
|
|
||
| - name: "Run squizlabs/php_codesniffer" | ||
| run: "vendor/bin/phpcs -q --no-colors --report=checkstyle | cs2pr" | ||
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.
Are changes like this really needed? I'd rather keep it simple and only add quotes where required (e.g. strings with special characters, numeric strings that must remain strings, etc.).
This file will be maintained by humans who will not see the need to add quotes everywhere.
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 trying to maintain consistency across repos
https://github.com/doctrine/orm/pull/8116/files
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.
So what's the reason why we want to have all strings quoted in all repositories?
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 suspect inspiration from https://github.com/ergebnis/php-library-template/blob/master/.github/workflows/integrate.yaml
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's just coding style, and that repo enforces it:
https://github.com/ergebnis/php-library-template/blob/master/.yamllint.yaml
I personally don't like everything quoted, as you point out it's not necessary all the time. but being consistent is the main thing i guess.
cc @localheinz
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.
@morozov
I am using
ibiqlik/action-yamllintto lint YAML files usingyamllint, and for consistency, I quote YAML strings.As @bendavies pointed out, it's a matter of taste and consistency. I have to admit that I dislike having to escape a bunch of characters when quoting.