-
Notifications
You must be signed in to change notification settings - Fork 23
Modernization #35
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
Modernization #35
Conversation
- Use PSR-12 - Move to WPCS 2 - Move to PHPCS 3.3 - Include VIP rules
As should already be there frm WPCS
Note how with PSR-12 many of the files headers on our codebase can break because PSR-12 does not allow declare(strict_types=1); on the same line of <?php.
szepeviktor
commented
Mar 31, 2020
PSR-12 does not allow
declare(strict_types=1);on the same line of<?php
...which is indeed very nice as it makes the code readable: 1 thing per line
szepeviktor
commented
Mar 31, 2020
A ruleset naturally has a lot of xml-s - from the previous millennia - please make sure these are checked.
For example see https://github.com/szepeviktor/phpcs-psr-12-neutron-hybrid-ruleset/blob/master/tests/xmllint.sh
PSR-12 does not allow
declare(strict_types=1);on the same line of<?php...which is indeed very nice as it makes the code readable: 1 thing per line
Readability is subjective, having a line for 5 characters <?php that likely the IDE will have in a different color plus a white line below does not increase readability for me.
But the thing about standards is that you don't have to like or agree... just follow.
The problem is that is a big backward compat issue for us (as in Inpsyde) because to follow this rule we will have to change almost 100% of our PHP files.
@gmazzap How about making a 1.0.0 release from this PR on? This way we would only have to adapt the composer.json of any project (instead of all php files) to not break all the CI?
@gmazzap How about making a
1.0.0release from this PR on? This way we would only have to adapt the composer.json of any project (instead of all PHP files) to not break all the CI?
@dnaber-de If this becomes the 0.15 we can adapt composer.json as well, from ~0.14 (or ^0.14) to 0.14.* and it will work anyway.
That said, I am fine this becomes 1.0.0 but I would make it "golden" without any usage in the real world. So maybe we can start with 1.0.0-beta1.
Another alternative is that we change the PSR-12 rule to accept declare in the first line with beside <?php.
I could do that. I already edited ConstantVisibilitySniff to don't show any warning if the project support PHP 7.0.
I don't give version numbers any meaning beside that defined in semantic versioning. So if we plan a proper public testing phase including a time plan for reviews and comments then a beta version is a a good choice. But if not, we should just go with 1.0.0. If we missed something, or find a bug we just release another version according to semver.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed the small bug in FixtureContentParser which results in all tests pass now. I did not dig into any detail but as you said standards are not there to like or dislike they just exist to follow them. Thanks for your effort on moving this forward.
I've read the commits, open issues and tested this branch on several repositories and found it to be behaving as expected without any issues.
Uh oh!
There was an error while loading. Please reload this page.
This brach if merged will:
Modernize the codebase:
Cleanup:
PHPCSAliases.phpthat's no more necessaryargandconfigfromruleset.xmlImprove:
Overall, merging this PR will fix:
So basically all open issues.
Before merging this has to be tested in the real world, that is, the updated repo as to be used to check a few WP codebases.
When merged we can release v0.15 which could be the 1.0-beta.