We should run phpcs against itself and make sure it passes.
Description
Description
Details
Details
| Subject | Repo | Branch | Lines +/- | |
|---|---|---|---|---|
| Make mediawiki/tools/codesniffer pass phpcs | mediawiki/tools/codesniffer | master | +80 -58 |
Related Objects
Related Objects
Event Timeline
Comment Actions
Change 214792 had a related patch set uploaded (by Polybuildr):
[WIP] Make phpcs pass against itself
Comment Actions
One common issue is the names of classes. For one, they're not in CamelCase and for another, they're too long, so they exceed line length. What should we do about them?
Comment Actions
Okay, so I've fixed most of the issues. There are 3 outstanding ones that need to be fixed and need some sort of consensus:
- The class names are of the form MediaWiki_Sniffs_AlternativeSyntax_AlternativeSyntaxSniff and so don't meet CamelCase standards, plus the lines containing these names are too long.
- There are URLs in the comments on top of the file, such as line 4 of TestHelper.php that are extremely long.
- Obviously, the *_fail.php tests fail. If we make this a voting job in zuul/layout.yaml (integration/config), then it will always fail. Not sure how this should be handled.
Comment Actions
So, point 3 from above was handled by adding *_fail.php to the --ignore parameter of phpcs.
I've uploaded a new patch set and added the test in composer.json. You can see the failures in the console output. That leaves only two outstanding issues, the class names and the long URLs.
Comment Actions
Alright, so based on @Addshore's advice, I split the class declaration lines into two and that's reduced a lot of errors.
There are only 2 specific issues remaining:
- The class names are in MediaWiki_Sniffs_AlternativeSyntax_AlternativeSyntaxSniff-like format (similar to upstream) and not in CamelCase as the standard requires.
- Some really long URLs still exceed line length.
The first is an error, the second is a warning. However, phpcs returns a non-zero status code even when there are only warnings, so if warnings are expected, it can't be a voting job (if I understand jenkins' work correctly).
Comment Actions
Aand we're down to our last error. The github URLs have been shorted using git.io and a #fragment has been removed from a URL. Line lengths are now fine.
Concerns left:
- Class names not being in CamelCase (the only remaining issue, then the repository will pass phpcs checks :D)
- Figuring out a generic convention to shorten URLs.
- The HHVM test failing (T100544) is causing the HHVM test to fail here too. I'm not exactly sure why... but both the die() that @Legoktm added in https://gerrit.wikimedia.org/r/#/c/214901/ and the Phar::running fatal error can be seen in the console output.
Comment Actions
Done. :D The codesniffer respository now passes its own tests. I'll upload a new patch now (without the "[WIP]" in the commit message) after reviewing my changes once.
Please do review the patch - it would be great if we could get this done and then maybe release a new version of the codesniffer tool.