localgovdrupal / localgov_shared_workflows

GNU General Public License v2.0
0 stars 0 forks source link

Add new use_module_phpstan_config option #1

Closed rupertj closed 2 months ago

rupertj commented 5 months ago

This change adds a new optional boolean input called use_module_phpstan_config. It's defaulted to false, which will use the phpstan.neon from the project.

If it's set to true it'll use phpstan.neon from the module under test. This lets module authors set the level to a higher one if they wish, etc.

stephen-cox commented 4 months ago

@rupertj I'm just wondering why you want to include you're own PHPStan config in publications? Ideally all LGD modules use the same rules, so if you have improvements/changes can they be applied to all repos?

rupertj commented 4 months ago

@stephen-cox I want to run a higher level than the default. IME the default doesn't catch many errors. Running at a higher level makes phpstan much more useful.

I'd be very happy if we turned the default level for every module up to something higher, but then we'd have to fix a lot of things all at once. Doing it this way lets module maintainers do it on their own terms.

stephen-cox commented 4 months ago

I'd be very happy if we turned the default level for every module up to something higher, but then we'd have to fix a lot of things all at once. Doing it this way lets module maintainers do it on their own terms.

It would be nice if we could flag tests in Github as non-failing and have them show a warning, that way we could roll out stricter PHPStan testing as a separate job and then fix issue as we have time.

This is possible in Gitlab, but doesn't seem straightforward in Github. There's a continue-on-error setting that should allow the rest of the jobs to run and report back, but I don't think there's a workflow warning state to highlight a non-required job has failed in a pull request (https://docs.github.com/en/rest/commits/statuses).

finnlewis commented 4 months ago

@millnut want to chip in on this one?

millnut commented 4 months ago

Yep I'm happy with this functionality I think it's handy to have modules where the maintainers want a higher level of phpstan checks and have the option to do so. As it's optional it just means modules with higher levels have less to fix in future if they choose a higher level of PHPStan checks.

Currently, for visibility this is the breakdown of issues/errors across our modules when increasing levels, based on Drupal core's plans:

We can then over time move to 2 and then 5 and then follow Drupal core's eventual plan to move to level 9.

The downside is surfacing the errors if we go higher on an optional workflow step, which @stephen-cox is correct doesn't work in GitHub (see https://github.com/orgs/community/discussions/15452) as well as GitLab handles this.

It still might be worthwhile to have this optional step targetting level 5 which doesn't fail a build but is used for monitoring purposes as a good goal to aim for going forward. This level 5 continue-on-error check could just sit at the project repo level for now and not be part of the shared workflow to not confuse new contributors.