Output PHP_CodeSniffer autofix diff in CI

[mvorisek]

Make constributions easier by providing easy to read autofix diff.

[greg0ire]

Can you please demonstrate the result on another repository?

[mvorisek]

Can you please demonstrate the result on another repository?

Here mvorisek/sql-formatter#1 is a PR demonstrating the behaviour.

The cs2pr behaviour is kept and newly an autofixable diff is output into the GHA CI - https://github.com/mvorisek/sql-formatter/actions/runs/9133275839/job/25116393429?pr=1#step:5:17. Such diff is very useful.

[greg0ire]

I'm skeptical: when there are autofixable errors, then people should manually apply that diff… they should IMO just run vendor/bin/phpcbf. I don't see in what cases seeing this diff can be useful.

[mvorisek]

I'm skeptical: when there are autofixable errors, then people should manually apply that diff… they should IMO just run vendor/bin/phpcbf. I don't see in what cases seeing this diff can be useful.

Altrough I made many Doctrine contributions already, for smaller contributions, I do not have phpcs very often installed. The "text output of cs2pr" is sometimes hard to understand so I would really, really appreciate if the diff can be shown, as the diff is very easy to understand what changes should be done in order to make CI happy.

[greg0ire]

I do not have phpcs very often installed.

How is that possible? Do you clone the repository every time? Or do you nuke your vendor directory after each contribution? It's one composer install away… Anyway, ideally contributors would run git hooks before pushing and that job should never go red. I really prefer to push them towards running checks locally instead of adding notifications for PRs with failing builds to my inbox.

[mvorisek]

I do not have phpcs very often installed.

How is that possible? Do you clone the repository every time? Or do you nuke your vendor directory after each contribution? It's one composer install away… Anyway, ideally contributors would run git hooks before pushing and that job should never go red. I really prefer to push them towards running checks locally instead of adding notifications for PRs with failing builds to my inbox.

There are thousand reasons. Changes might be done using GH web IDE, from enviroments /wo composer, local PHP version might be too low (for ex. in my work we use still PHP 7.4 on some Windows machines), ...

In anycase, I run into these situations myself, thus I propose this CI improvement which I find useful.

[greg0ire]

The code looks good, I'd like to get input by other Doctrine maintainers about the usefulness. My very personal opinion is that we should encourage people not to rely on the CI for checks so as to get as few notifications about PRs with failing builds as possible.

[ostrolucky]

doesn't this double execution time?

[mvorisek]

doesn't this double execution time?

Even /wo cache the execution is fast - ~6 secs for Doctrine DBAL, ~9 secs for Doctrine ORM.

[ostrolucky]

does it make sense to do --no-colors for diff output?

[SenseException]

does it make sense to do --no-colors for diff output?

Wouldn't colors leave a bunch of console-color noise like [30m in the output?

[mvorisek]

does it make sense to do --no-colors for diff output?

I agree colored output it better for UX - I commited the change, here is an example: https://github.com/mvorisek/sql-formatter/actions/runs/9135074458/job/25121808121?pr=1#step:6:17