async icon indicating copy to clipboard operation
async copied to clipboard

Published 20 hours ago • verified publisher icon reactphp

[4.x] Set up PHPStan on GitHub Actions

Open nhedger opened this issue 3 years ago • 2 comments

This PR sets up PHPStan to run on GitHub Actions, as discussed in discussions#469.

Overview

  • [x] Sets up PHPStan to run on GitHub Actions on PHP 8.1 only
  • [x] Configures PHPStan to run the analysis on the src and tests folders
  • [x] Generates the baseline so that static analysis passes immediately

Baseline

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.x and 2.x branches.

nhedger avatar Sep 17 '22 06:09 nhedger

@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.

SimonFrings avatar Sep 28 '22 12:09 SimonFrings

I've pinned the version of PHPStan to the current latest.

nhedger avatar Oct 06 '22 14:10 nhedger

@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!

clue avatar Apr 24 '23 10:04 clue

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.

nhedger avatar Apr 24 '23 10:04 nhedger

For the reference: We've recently merged PHPStan on max level into 4.x with #76 and backported to 3.x with #77. Unlike the changes in this PR, we've gone through the project to fix all PHPStan errors encountered so we don't have to rely on a baseline. We'll go ahead with this and apply PHPStan on max level to all our projects in the future.

Thank you very much for your contribution and sparking this discussion! :+1:

clue avatar Jun 23 '23 10:06 clue