Nitpicking over Code Standards with Nitpick CI

There are many ways to make sure your code respects a given code standard – we’ve covered several before. But enforcing a standard team-wide and making sure everyone knows about mistakes before they’re applied to the project isn’t something that’s very easy to do. Travis and Jenkins can both be configured to do these checks, but aren’t as easygoing about it as the solution we’re about to look at: Nitpick CI.

Nitpick CI is a dead simple single-click tool for making sure submitted Github pull requests adhere to the PSR-2 standard. Unfortunately, Nitpick currently only works with these two specific (but popular) vectors – only Github, and only PSR-2. It’s free for open source projects, so let’s give it a try.

Bootstrapping

To test Nitpick, we’ll create a brand new repository based on thephpleague/skeleton and pretend we’re building a new PHP package. The skeleton already respects PSR-2, so it’s a perfect candidate for an invalid PR. Feel free to follow along!

git clone https://github.com/thephpleague/skeleton nitpick-test
cd nitpick-test
find . -type f -print0 | xargs -0 sed -i 's/:author_name/Bruno Skvorc/g'
find . -type f -print0 | xargs -0 sed -i 's/:author_usernamename/swader/g'
find . -type f -print0 | xargs -0 sed -i 's/:author_website/http:\/\/bitfalls.com/g'
find . -type f -print0 | xargs -0 sed -i 's/:author_email/bruno@skvorc.me/g'
find . -type f -print0 | xargs -0 sed -i 's/:vendor/sitepoint/g'
find . -type f -print0 | xargs -0 sed -i 's/:package_name/nitpick-test/g'
find . -type f -print0 | xargs -0 sed -i 's/:package_description/nitpick-test package for a SitePoint tutorial/g'
rm CONDUCT.md
rm -rf .git

The above commands clone the skeleton, replace placeholder values with real values, and delete some files we don’t need. The project is now ready to be committed and pushed online (as long as you created a repo on Github).

git init
git remote add origin YOUR_ORIGIN
git add -A
git commit -am "Initial commit"
git push -u origin master

We can now tell Nitpick to track it.

Configuring Nitpick

To set up Nitpick, we really only have to register for it through Github’s Oauth:

Once authorized to look at an account’s repos, Nitpick will offer a list with a single “Activate” button next to each.

There’s even a handy search field for instantly filtering the repos list if you have hundreds. One click on the Activate button, and that’s all it takes. Nitpick is now watching the project for PRs and automatically scanning them. Let’s give it a go!

A Non-Code PR

First, let’s test this on non-code PRs and see how Nitpick will react. We deleted the CONDUCT file in the first steps of this tutorial, but we failed to delete the reference to it from the README file. There’s also a “note” at the top of the README file that needs to be removed.

We can do these changes in the online UI by clicking the edit button on the README file:

To actually create an inspectable PR, we tell Github to create a new branch and a new pull request:

As expected, the PR is left untouched – nothing really happens and we can merge it. The file has no code to inspect, so no notifications are issued.

A Valid Code Pull Request

Now, let’s add a change to the sample controller. The change should be minimal, just to see if Nitpick will do anything. For simplicity, we can use the web UI again.

We can change the content of src/SkeletonClass.php to:

< ?php

namespace League\Skeleton;

class SkeletonClass
{
    /**
     * Create a new Skeleton Instance
     */
    public function __construct()
    {
        // constructor body
    }

    /**
     * Friendly welcome
     *
     * @param string $phrase Phrase to return
     *
     * @return string Returns the phrase passed in
     */
    public function echoPhrase($phrase)
    {
        $phrase .= "- and here is a suffix!!";
        return $phrase;
    }
}

And again, nothing happens. Is Nitpick even working?

An Invalid Code Pull Request

Now, let’s write some PSR-2 unfriendly code on purpose. PSR-2 states:

There MUST NOT be a hard limit on line length; the soft limit MUST be 120 characters; lines SHOULD be 80 characters or less.

and

Visibility MUST be declared on all properties and methods; abstract and final MUST be declared before the visibility; static MUST be declared after the visibility.

While there are many more rules, these seem easy enough to break. Let’s change the contents of src/SkeletonClass.php to:

<?php

namespace League\Skeleton;

class SkeletonClass
{
    $someProp = "foo";
    public $someOtherProp = "bar";

    /**
     * Create a new Skeleton Instance
     */
    public function __construct()
    {
        // constructor body
    }

    public final function thisIsNotPsr2() { echo "Hello!"; }

    /**
     * Friendly welcome
     *
     * @param string $phrase Phrase to return
     *
     * @return string Returns the phrase passed in
     */
    public function echoPhrase($phrase)
    {
        $phrase .= "- and here is a suffix!! But it doesn't end there - we have here a very, very long line that's actually a bunch of text. Technically, it should be broken up or something I guess.";
        return $phrase;
    }
}

We’ve added two properties, of which only one is defined according to rules. The other is missing its visibility declaration. Furthermore, we added a technically valid but standards-invalid method which defines its finality after the visibility, and then has its entire body in the same line as the method’s declaration. Lastly, we extended the previously added suffix to an extreme length.

Once we submit the PR, we can see things are a bit different:

Nitpick displays the mistakes inline, specifically pointing out what’s wrong – whether it’s just one offense or multiple ones as in the case of our added method.

Due to Nitpick commenting on the PR directly as if a user were doing it, notifications are also sent via email, so it’s nearly impossible to miss them:

One thing it missed, however, is the line length. It didn’t react to it at all. This is because under the hood, Nitpick uses PHP CodeSniffer and it’s PSR-2 preset, and what ever PHPCS considers valid, Nitpick agrees with. On the specific line length issue, PHPCS considers it merely a warning, which we can see if we run it manually on our code:

composer global require squizlabs/php_codesniffer
~/.config/composer/vendor/bin/phpcs --standard=PSR2 src/SkeletonClass.php

After inspection, it’s not like Nitpick will block the merge attempt. We can still go ahead and merge the PR as usual. The inspection messages, however, are there to stay for public record until we get them fixed. Let’s do that now. On the command line, we’ll clone that PR’s branch:

git fetch origin
git checkout -b Swader-patch-3 origin/Swader-patch-3

We then make the required edits, changing the SkeletonClass.php‘s contents to:

<?php

namespace League\Skeleton;

class SkeletonClass
{
    public $someProp = "foo";
    public $someOtherProp = "bar";

    /**
     * Create a new Skeleton Instance
     */
    public function __construct()
    {
        // constructor body
    }

    final public function thisIsNotPsr2()
    {
        echo "Hello!";
    }

    /**
     * Friendly welcome
     *
     * @param string $phrase Phrase to return
     *
     * @return string Returns the phrase passed in
     */
    public function echoPhrase($phrase)
    {
        $phrase .= "- and here is a suffix!! But it doesn't end there - we have here a very, very long line that's actually a bunch of text. Technically, it should be broken up or something I guess.";
        return $phrase;
    }
}

After committing and pushing with:

git add -A
git commit -m "Fixed nitpicks"
git push origin Swader-patch-3

… the only feedback we get about having fixed the mistakes is the fact that Nitpick’s previous comments on the PR are now outdated:

Our PR is now ready to be merged!

Conclusion

While Nitpick is, admittedly, very specific in its use case, it’s also very good at it – it does one thing, and does it well. A single click is all it takes to configure it, and the entire team benefits from advanced standard inspections which can now be left out of other CI tools like Jenkins and Travis, instead letting them focus on actual tests. Being free for open source and incredibly easy to set up, there’s truly no reason not to give it a go.

Granted, there are some nitpicks with Nitpick:

• due to Nitpick using PHPCS under the hood, we’re constrained by what the tool can do. Not only that, we’re also constrained by the tool’s interpretation of the PSR-2 rules. There’s no way to customize the ruleset, or to define a different implementation of the standard (for now).
• when mistakes get introduced, there’s no blocking indicator in the PR. CI tools like Travis and Scrutinizer do this really well in that they work together and use the PR’s readiness indicator to mark a build fail or pass. While code standards have no real effect on a build’s technical realization passing or failing, it’d be nice to have some more visual feedback in the spirit of those tools.
• when mistakes get fixed, there’s no clear indication of the issues having been rectified. I like Scrutinizer‘s approach there – clear indications in the PR, and an email saying some issues were fixed. This gets sent to the entire team, so everyone’s immediately on the same page.

That said, I’ll definitely activate Nitpick on all my open source projects – there’s just no reason not to. As a heavy PSR-2 user, I can leave the standards checking to Nitpick and forget all about it in tools like Travis and Scrutinizer, instead letting those tools focus on what matters – code inspection and tests.

What do you use for code inspection? Do you rely on external tools, or local tools only? Maybe both? Let us know!