Configure scrutinizer for coding standards checks

[pfrenssen]

Experimenting with Scrutinizer CI. I have set it up in my fork. See the first results here: https://scrutinizer-ci.com/g/pfrenssen/og/?branch=8.x-1.x

[amitaibu]

Cool stuff

On Aug 9, 2016 8:11 PM, "Pieter Frenssen" notifications@github.com wrote:

Experimenting with Scrutinizer CI. I have set it up in my fork. See the first results here: https://scrutinizer-ci.com/g/

pfrenssen/og/?branch=8.x-1.x

You can view, comment on, or merge this pull request online at:

https://github.com/amitaibu/og/pull/301 Commit Summary

• Configure scrutinizer for coding standards checks.

File Changes

• A .scrutinizer.yml https://github.com/amitaibu/og/pull/301/files#diff-0 (4)

Patch Links:

• https://github.com/amitaibu/og/pull/301.patch
• https://github.com/amitaibu/og/pull/301.diff

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/amitaibu/og/pull/301, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHrCxQVSDKLaPhYypn7tuIeAPOjt24gks5qeLNkgaJpZM4JgTWa .

[pfrenssen]

Little update about how this is going: configuring Scrutinizer CI is not really straightforward. It does a lot of checks which are nonsensical for a Drupal project. These can be filtered out by clicking in the UI, but it does not seem to be possible to configure this in the .scrutinizer.yml file.

It also seems to mark all tests as green, since it considers our current code quality to be "good enough". There is some documentation about how to mark a build as failed (https://scrutinizer-ci.com/docs/configuration/build_status) - looking into that now to mark any PHPCS error as a failure.

[pfrenssen]

I'm not going to continue to try to get Scrutinizer to conform to our standards. It really tries hard to autoconfigure things so it can give some recommendations. I cannot seem to disable it after trying for hours.

I've now tried a different service: ContinuousPHP, it literally took 5 minutes to configure it, and it's working out of the box: it is checking coding standards and correctly failing the build.

[amitaibu]

Thank you for doing this!

[pfrenssen]

My coding style fixes have introduced some bugs it seems.

@amitaibu I suppose you don't like these kinds of changes:

-    if ($permissions) {
+    if (!empty($permissions)) {

This were suggestions by Scrutinizer CI - if a variable contains an array it suggests to use empty() to check if it is empty, so that the value is not implicitly cast to a boolean. If I remember correctly you prefer the shorter version?

Are there also other cleanups that I have done that you think don't have much value?

My next plans for this PR are:

• Obviously get the tests green again.
• Try to see if I can get a simplified version running on Travis CI, which runs in a separate instance, and doesn't require huge changes to the .travis.yml file. ContinuousPHP was easy to set up and runs fine, but having a single service is still preferable. Also ContinuousPHP needs to be set up using the graphical interface which is less than ideal. If it supported configuration through a .continuousphp.yml file then it would be ideal, but the UI configuration makes it less easy to adopt in people's forked repos.
• I intend to rebase the commits in this branch so that we don't have a zillion similar commits. Ideally it should only have 2-5 commits in total, one for each separate action: 'fix scrutinizer warnings' - 'update Travis config' etc.

[amitaibu]

I suppose you don't like these kinds of changes:

Yes, I don't like them, as they might hide errors of wrong initializations of values - I prefer to get an error/ notice - and fix the underlying issue, instead of wrapping all statements with empty().

IMO, don't loose to much time on the rebases - we should use you time and talent on more important issues ;)

[pfrenssen]

Closing in favor of https://github.com/Gizra/og/pull/173