Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add "composer fix" to run php-cs-fixer and phpcbf. #3426

Merged
merged 3 commits into from Feb 20, 2024

Conversation

EreMaijala
Copy link
Contributor

Includes a change to phpcbf to run it with the output of phpcs, because phpcs is much faster with the cache.

Includes a change to phpcbf to run it with the output of phpcs, because phpcs is much faster with the cache.
Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @EreMaijala, this is great! Just one small comment. (I also committed a fix to adjust some minor indentation inconsistencies).

build.xml Outdated
</filterchain>
</property>
<!-- Finally, run phpcbf: -->
<exec command="${srcdir}/vendor/bin/phpcbf --standard=${srcdir}/tests/phpcs.xml ${phpcbf_filelist}" escape="false" passthru="true" checkreturn="true" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we might want to change checkreturn to "false" here -- otherwise, when phpcbf fixes something, the process behaves as though something has gone wrong due to the return status. Any objection?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No objection. It just was that way before and I didn't touch it or give it a thought at all. Changed in the last commit.

@demiankatz demiankatz added the tooling pull requests that change project tooling label Feb 16, 2024
@demiankatz demiankatz added this to the 10.0 milestone Feb 16, 2024
Copy link
Contributor

@bbusenius bbusenius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to give an approval here. I don't know much about php-cs-fixer and phpcbf but in reading about them they look good. Speed increases are always welcome and I don't see any problems here.

Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works well for me now; thanks, @EreMaijala!

(I'm not entirely sure why a non-zero return code became a problem now when it was not an issue in the past, but maybe the added arguments to phpcbf change the way its return values operate).

@demiankatz demiankatz merged commit 118a324 into vufind-org:dev Feb 20, 2024
7 checks passed
@demiankatz demiankatz deleted the dev-composer-fix-action branch February 20, 2024 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tooling pull requests that change project tooling
Projects
None yet
3 participants