[4.x] Set up PHPStan on GitHub Actions#64
[4.x] Set up PHPStan on GitHub Actions#64nhedger wants to merge 6 commits intoreactphp:4.xfrom nhedger:ci/phpstan
Conversation
| - name: Checkout | ||
| uses: actions/checkout@v3 | ||
| - name: Set up PHP | ||
| uses: shivammathur/setup-php@v2 |
There was a problem hiding this comment.
nit: Remove names for consistency with other actions?
| - name: Checkout | |
| uses: actions/checkout@v3 | |
| - name: Set up PHP | |
| uses: shivammathur/setup-php@v2 | |
| - uses: actions/checkout@v3 | |
| - uses: shivammathur/setup-php@v2 |
There was a problem hiding this comment.
To be honest I'd rather we add names to the other steps for consistency x)
| - name: Install Composer dependencies | ||
| run: composer install | ||
| - name: Run static analysis | ||
| run: phpstan analyse |
There was a problem hiding this comment.
nit: Remove names for consistency with other actions?
| - name: Install Composer dependencies | |
| run: composer install | |
| - name: Run static analysis | |
| run: phpstan analyse | |
| - run: composer install | |
| - run: phpstan analyse |
| - | ||
| message: "#^PHPDoc tag @param for parameter \\$tasks contains generic type React\\\\Promise\\\\PromiseInterface\\<mixed, React\\\\Async\\\\Exception\\> but interface React\\\\Promise\\\\PromiseInterface is not generic\\.$#" | ||
| count: 3 | ||
| path: src/functions.php |
There was a problem hiding this comment.
I understand this was generated as a baseline, but how is this going to affect the build status once Promise actually adds @template definitions via reactphp/promise#227? It's my understanding this would start failing once this error is no longer matched? Does this mean we should fix such errors upstream first?
(Same applies to other occurrences)
There was a problem hiding this comment.
I must admit this did not cross my mind. I guess fixing the error upstream would be sensible.
Co-authored-by: Christian Lück <[email protected]>
Co-authored-by: Christian Lück <[email protected]>
Co-authored-by: Christian Lück <[email protected]>
|
@nhedger I really like this idea, I'm not sure what's the best way to do this. I see a problem with future contributions to the project. There can (and will be) cases where someone files a PR and something unrelated to their changes shows an error due to a new PHPStan version. We could now tell him to generate a new baseline and that's it, but it feels wrong to see upgrading unrelated parts as the contributors job. It also mixes the actual changes in this persons PR with unrelated changes that needs to be handled in a different pull request. I think it's in everyone's best interest to keep the contribution part as easy as possible, to avoid unnecessary confusion and frustration on the contributors side and keep the review process smaller. I think the way to avoid this problem is to lock the used PHPStan version as @clue suggested above. |
|
I've pinned the version of PHPStan to the current latest. |
|
@nhedger Was this PR closed intentionally (along with related ones as per https://github.com/orgs/reactphp/discussions/469)? I've also started looking into PHPStan on max level with no baseline a while ago, would you like to keep working on this PR or do you think it makes sense for me to file my PR in comparison? In either case, thank you for your patience and for looking into this! |
|
Ah sorry, it got closed because I cleaned up my forks. Completely forgot to check for open PRs. Not planning to work on this in the near future, so feel free to file your own PR in the meantime. |
|
For the reference: We've recently merged PHPStan on max level into Thank you very much for your contribution and sparking this discussion! 👍 |
3 participants
This PR sets up PHPStan to run on GitHub Actions, as discussed in discussions#469.
Overview
srcandtestsfoldersBaseline
Because this PR aims to set up PHPStan and not address the errors it reports, I've generated a baseline to make the pipeline succeed. We'll then be able to incrementally fix the problems in future PRs.
Once this has been merged, I'll create PRs for the
3.xand2.xbranches.