-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 PHP CS Fixer as a formatting tool and lint-stage during github CI #2521
base: master
Are you sure you want to change the base?
Conversation
Thank you :) this PR makes everything look much neater; I'll try to get it merged soon, but it's another long review process since it touches so many files. This is modifying some autogenerated code, so I'd like to modify the code generator before merging this so that it doesn't give us warnings every release with newly autogenerated code. Since this is mostly whitespace changes, I think it'll be much faster though. |
Actually, I could add it in as part of this PR, that probably makes more sense. Can you grant me rights to edit it? |
Glad to hear you see value in my PR :-). I already checked the "Allow edits and access to secrets by maintainers" and now I invited you as a collaborator on my project, so you should be able to contribute to it (once you accept the invitation). Hats off to you if you are willing to go through the whole PR. If you want to make sure I did not manually add something nefarious in my second commit, feel free to drop it and recreate it. Only thing I did there was run the tool so I don't take credit for that commit at all. We could also do it in increments if that is easier to process. To do this, we would simply need to modify |
Hi @Zombaya, Thanks again for the contribution and apologies for the delay— we were a bit busy with some major functionality and are just now finding the time to focus on community PRs. Could you rebase? Interested in resuming our review of this. |
9a30f7d
to
e04458e
Compare
Hi @stobrien89 , It's a busy time at work so I wasn't able to get to it immediately. I've done the following:
You can now run:
To review this, I'd focus on verifying the first commit and generating the changes of the second commit yourself, so you are certain I did not manually tamper with the files. I would not want to be in your shoes if you are going to review all those changes one by one. I'd be imppressed if you were to pull that one off! |
I noticed I forgot to update contributing.md to reflect the change of codestyle from PSR-12 to PER and to update the lint-CI-job to output the new composer commands to fix your codestyle. I ammended the commit and pushed again. |
Squashed commits ---------------- * fb76eba 2022-06-13 Zombaya Add php-cs-fixer as tool to project. Installed according to best practice in README of php-cs-fixer. * 549ceae 2022-06-14 Zombaya Add pipeline-check for valid codestyle using php-cs-fixer * 11812f2 2022-09-13 Zombaya Update CONTRIBUTING.md and set version of php-cs-fixer fixed to current latest version * 01cb480 2022-09-13 Zombaya Autoformat src and tests to PSR-12 using php-cs-fixer * e5e6e9599 2022-09-13 Zombaya Add release-file * Modified codestyle from PSR-12 to PER * Added composer-scripts to more easily run php-cs-fixer
During review of #2403, errors in formatting/code style were sometimes spotted by the reviewers and had to be manually fixed.
I strongly believe this is something better suited for a tool to handle. I feel this will lessen the burden on both
contributors and reviewers as the formatting of the code will no longer be a point of discussion.
Description of changes
tools/php-cs-fixer/vendor/bin/php-cs-fixer fix -v
and autoformatted all code in a seperate commitRationale of certain choices
PHP CS Fixer as formatting tool
There are other tools which can be used as a formatting tool (such as PHP_CodeSniffer). Since I personally had the most experience using PHP CS Fixer and because the tool was already mentioned in CONTRIBUTING.md, I chose to use this tool specifically.
Fixed version
I opted to use an exact version in
composer.json
intools/php-cs-fixer
to avoid formatting-conflicts resulting from different version of the tool used.This is the latest version available and supports php 7.4|8.*.
I believe we should not support older versions here since this is only used to develop the library, not by end-users of the library.
As a downside, this means that occasionally the version will have to be manually bumped in
tools/php-cs-fixer/composer.json
to keep the formatting-standard up to date.PSR-12PER as standardSince PSR-standards were already referenced in CONTRIBUTING.md, PSR-2 has been deprecated in favor of PSR-12 since 2019-08-10, which is now deprecated in favor of PER and I feel the larger PHP-world is migrating to PER, I found this to be the most suitable standard to start with.
Possible improvements
tools/php-cs-fixer/vendor/bin/php-cs-fixer
invendor/bin
to make it easier to run the tool. I opted not to do this since I feared issues when somebody would use windows or mac to develop this library.Merge-strategy
To review my actual changes, I'd review my first commit separately from the second.
The first commit holds all relevant changes to the configuration of the project. The second commit only contains changes made by PHP CS Fixer, so only contains formatting changes.
Because the autoformat changes so many files, I'd not merge this PR directly.
Once some commits will have been committed to master, this PR will have mergeconflicts which will be hard/tedious to fix manually.
Instead, I would rebase on master, skip/drop my latest commit containing the automatic formatting of all files and recreate that commit manually.
Post Scriptum
Feel free to let me know if you feel this wouldn't be of benefit to the project or that this PR needs some tweaks before it can be merged.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.