Cleaned up code with the help of php-cs-fixer

[frastel]

I have seen a lot of tabs/spaces mixup in the code and some incorrect indention. So integrated friendsofphp/php-cs-fixer and let it do its work.

Additionally I pumped PHPUnit version to 4.5 as it is used on Travis. The testsuite fails with the defined PHPUnit 3.7 version.

This step is pre-work for my next fix on this lib.

[shmax]

Interesting. So how do we keep the code tidy going forward? Are we supposed to run something from the command line? If so, should you add something to the README? If it's meant to be automatic, can it be integrated with travis? If not, shoud a linter be integrated with travis?

[frastel]

I executed this command before the commit vendor/friendsofphp/php-cs-fixer/php-cs-fixer fix JBBCode/. The files are changed automatically and one has to commit the changes afterwards. Doing this step during CI would be too late though. However linting would be possible with phpcheckstyle/phpcheckstyle during CI but then it has to be defined which coding standard should be complied. I would suggest PSR-2 as it is widely-used.

[shmax]

Okay, but I still don't understand how this is going to be enforced or encouraged going forward. You felt strongly enough about the formatting to do a single pass with this tool you've found, but if you don't wire up a linting step to the build process or even mention it in the README as a best practice then what is the point of adding this tool to the project at all? Going forward, no one will know that you expect them to follow any kind of standard, and the php-cs-fixer will just become one of those composer includes that people puzzle over until someone has the bright idea to remove it as part of a drive to clean up unused dependencies.

[shmax]

@DaSourcerer any thoughts?

[DaSourcerer]

Regarding the whitespace fixup: +1,000! I think @jbowens went for PSR-2 as well. Funny how this wasn't fixed for such a long time. And TBH: I may have submitted a similar PR myself :smile:

wrt the phpunit version: All fine by me. I had some issues with versions larger than 4.5 (cf #74) as the mockbuilder api has changed.

[DaSourcerer]

@shmax:

I still don't understand how this is going to be enforced or encouraged going forward.

Travis isn't the only tool for code quality. I think scrutenizer can also check for adherence to given coding standards.

[jdreesen]

Maybe try StyleCI ? It's free for open source, of course. I haven't used it myself yet, though.

[DaSourcerer]

@jdreesen: Noted. Though I were a bit reluctant as it is a service for a singular purpose. Scrutenizer CI also features code quality analysis and whatnot in addition to coding standard compliance checks.

[jdreesen]

Right, ScrutenizerCI does much more and might be the better service to use. Although I've got no experience with using it as a CS fixer.

[frastel]

@shmax I think the CS fixer or linter is not really a mandatory tool in CI. However it helps a lot to clean existing code. I know how frustrating it could be getting a red build just by commiting one incorrect indended line. Its just cosmetics and does not introduce any bug.

More important is the commitment to any coding standard. It does not matter if this coding standard is followed manually, with the help of the IDE, some tool or just code reviews. I do not want to enforce the usage of this tool right now. However I wanted to start my contribution from cleaned files.

[shmax]

@frastel Fair enough. LGTM

[DaSourcerer]

I think the CS fixer or linter is not really a mandatory tool in CI.

FullACK. It is a useful tool for QA, but keeping a PR from being pulled is a bit too dramatic, indeed.