amitaibu / og

A fork to work on OG8
https://github.com/Gizra/og
29 stars 16 forks source link

WIP: Adding the script for the code sniffer running. #260

Closed RoySegall closed 8 years ago

pfrenssen commented 8 years ago

What is this? Is this intended for a git hook?

Expecting people to add [skip sniff] to their commit messages is not really great. This would encourage people to actually add this to their messages. A commit message should describe the code change that a particular commit contains, it should not have any test infrastructure directives.

pfrenssen commented 8 years ago

Here's the git hook that I built for code sniffing the projects at the European Commission: https://gist.github.com/pfrenssen/498fc52fea3f965f6640

This is a standalone component that can be included in composer.json, so we don't need to maintain this ourselves. There's no need to reinvent the wheel ;)

I have a lot of experience with this stuff, I set up CI pipelines for dozens of projects over the past couple of years. One thing I learned is that you should do this on pre-push, not on post-commit. Doing it on post-commit will cause complaints from the developers because committing is so integral to people's workflows. They don't want to get slowed down by a code sniffer run in the middle of a coding session.

I'd like to help with setting this up but I will have to do it in my spare time. Next week I'm at Drupalaton, I will have plenty of time starting from Tuesday.

RoySegall commented 8 years ago

Yes. This should be a hook but I'm looking how to add this automatically. As I understood there is no way to do this and people will have to do this manually so I'm thinking on closing this one. Another option is to add a couple of commands to the read me file.

Expecting people to add [skip sniff] to their commit messages is not really great.

You do have the option to add [skip ci] in order to prevent from travis run every time you push therefor I don't think it's a bad idea.

amitaibu commented 8 years ago

Sniffer checks aren't expensive on time, so we don't need to provide a special skip. While a bit ugly I agree the skip ci is handy to prevent a long queue of tests.

But yeah, I think his can be closed in favor of a few words a link to the phpcs docs in the Readme

pfrenssen commented 8 years ago

Installing the git hook can be done automatically. In the project I linked I am leveraging the post-install-cmd hook from Composer to install the git hook. You need to run composer install to get PHP CodeSniffer installed anyway, and that also puts the hook in place.

Depending on the environment you work on the scanning can take longer than you might expect. We had cases where developers were using Windows, were working on a slow shared network drive, or were using a remote server with a slow connection. Even a delay of 1-2 seconds when committing was found to be very annoying, and people started disabling the git hook. On the other hand, pushing only happens rarely, typically when they are done with their work. Scanning on pre-push works really well in people's workflows.

amitaibu commented 8 years ago

Closing in favor of #301