Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.Sign up
Add PHP-CS-Fixer integration #1148
This PR aims to introduce coding-standards in the project.
The check is done in the lowest PHP version supported, to retain full compatibility.
To ease the build, the code-coverage is done to the latest PHP version supported.
The fix will be applied only when this PR will be reade to merge, so the diff of the PR is easier to review.
What coding standard is this? I gather it's not PSR-2. I've had trouble with php-cs-fixer in the past suggesting changes that are actually breakages (it's used in scrutinizer), and I see there is one phpdoc mistake in the suggested diff (adds a
Could you fix whatever is breaking the build.
The main rule activated is
Since version 2 PHP-CS-Fixer distinguishes safe fixers from risky ones. But what does the tool consider risky? Risky is when and only when a user overwrites default PHP behaviours, for example
The fixers proposed here don't introduce any breakage, even though some of them are risky; after ending this discussion/review I'll apply the fixer and you'll see no test failing.
The fixer applying this change is
This PR integrates coding standards check in the build to force commiters adhere to the standards, but I intentionally avoided applying them for now because I expected this topic to be discussed, and to show you what changes, so for now the build intentionally fails.
added a commit
this pull request
Sep 7, 2017
I've fixed just about everything flagged. The few things left are annoying - php-cs-fixer and PHPStorm seem to have different definitions of Symfony's coding standard. php-cs-fixer's approach is the more annoying, imposing inconsistent spacing on param descriptions, and requiring an enormous number of blank lines in docblocks - this is one reason I've disliked Symfony's standard in the past. PHPStorm doesn't seem to format params with phpdoc nullable types, like
There are a few of the examples with the trailing
As you can see - just silly.
I appreciate that you took this PR advice to fix the current code.
But my intention with this PR is wider.
Instead, in the commit 31493b6
The result is that the next commit from you or from contributors can be written without rules, and either you have to check them manually (boring and error-prone) or accept the code standard of someone else.
Moreover, I am a bit sad that you committed a part of this PR without actually discussing and merging this very PR. As a sign of gratitude for a contributor that spent his/her time improving your project, it is appreciated to discuss the topic and to merge the code proposed by him/her, so the contributor can become part of the code through commits authoring.
The code that follows this statement,
Hold on a sec! The intention is to accept your PR, and it's fully appreciated that you're contributing - however, since it needs to pass before committing, the things it's going to check need to be fixed beforehand, so that's what I started on. That way your PR will pass tests before merging, rather than after. This project already used PSR-2 as the coding standard, and nearly all the changes suggested by php-cs-fixer are for the Symfony standard; what's new is your config change to enforce the rules, which is very welcome.
The only thing that overlaps with your PR was the addition of the php-cs-fixer package in composer.json, which I'd added locally and committed by accident - I've pushed a reversion of that change, so it should apply cleanly again - if not, please rebase.
This is the code that caused the extra
and this is the diff php-cs-fixer suggests, using your config file:
phpdoc used to recommend using code or pre tags in docblocks for inline code examples (why these tags are there), and it's simply inappropriate to add a trailing full stop after it. That said, phpdoc now recommends markdown instead, so I've converted the code tags to use that, which avoids the problem, though see below for a similar complication.
For some reason it's also suggesting an incorrect change for a namespaced class that is imported correctly, as far as I can see:
There are some inappropriate capitalisation changes too:
This is flagged for the trailing
It doesn't accept a
You can't resolve that one by removing the blank line before the code block as it will fail to render the markdown, and it then suggests a trailing
I absolutely agree that code style should be enforced - but it's a bit pointless if the enforcement mechanism introduces errors of its own.
I'm sorry I didn't explained myself enough. It was never my intention to have the build passing after merging, it should never happen. The build process is there for this very reason: a PR needs to pass before merge. So why I am still leaving the build failing (for now)?
It would be much harder for you (the maintaner) to inspect the
Without immediately applying the fix:
So, when you'll tell me the configuration is ok to you, I'll apply the fix, the build will pass and then, and only then, you will agree whether or not merging the PR
About the config:
@@ @@ * - * @param string $host The hostname to connect to - * @param int|bool $port The port number to connect to - * @param int|bool $timeout The timeout value + * @param string $host The hostname to connect to + * @param int|bool $port The port number to connect to + * @param int|bool $timeout The timeout value * @param string $username * @param string $password * @param int $debug_level
That I consider valid, so if now the config is ok to you, I'll apply the fix, push a new commit, the PR should pass and you can merge it.
composer.lock is removed
I care a lot that you note the second commit of this PR removes
This is not an issue, and instead is good. Composer documentation says Commit Your composer.lock File to Version Control, but the statement only refers to applications, not to libraries.
If you commit the composer.lock on PHP 5.5, Travis will always use the locked dependencies to run the build, but on newer PHP versions you may miss new improvements or new changes, and you can't adapt to it as soon as they happen.
Can you explain what the difference is between the
I agree about the param label indenting being out - that's a difference between php-cs and PHPStorm for the Symfony standard - I'll report that as a bug in PHPStorm. It seems to be different when some params are missing labels - it only makes the params that have descriptions line up, and ignores the length of param names that don't have descriptions (i.e. it doesn't consider hypothetical descriptions). I'm not that keen on adding all that white space anyway - it leads to silly line wrapping if you have a short line length limit and long var names, making things distinctly less readable when you're looking at the code, and it makes no difference to rendered phpdoc output.
I'm familiar with committing
Symfony standards suppose users always use FQCN, and so the fixer adds the root backslash to avoid IDE like PHPStorm doing a mess. As you said, the fixer expects you to write
I've applied the fix and pushed the commit, if you like it the PR is ready to be merged.