Adds git hooks to skeleton

[NigelGreenway]

Description

Allows composer check-style to be ran on pre-commit and composer test on pre-push.

Motivation and context

For me, it is about bringing all engineers inline. At the end of the day we are only human and automation on things like this allows to concentrate on the problem to solve and reduce potential stress.

How has this been tested?

Ran ./vendor/bin/captinhook install and made breaking changes on code and tests to check the errors stop committing and pushing to remote.

Types of changes

What types of changes does your code introduce? Put an x in all the boxes that apply:

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

Go over all the following points, and put an x in all the boxes that apply.

Please, please, please, don't send your pull request until all of the boxes are ticked. Once your pull request is created, it will trigger a build on our continuous integration server to make sure your tests and code style pass.

• I have read the CONTRIBUTING document.
• My pull request addresses exactly one patch/feature.
• I have created a branch for this patch/feature.
• Each individual commit in the pull request is meaningful.
• I have added tests to cover my changes.
• If my change requires a change to the documentation, I have updated it accordingly.

If you're unsure about any of these, don't hesitate to ask. We're here to help!

[NigelGreenway]

Not sure if the hooks get pushed to remote? If not, should this feature be plugged into the prefill script?

[NigelGreenway]

Hey, I was just wondering if there are any updates on this being put in or not? Just want to make sure all is up to date before I resolve conflicts.

[colinodell]

Hey Nigel,

I really appreciate you taking the time to propose this change and am very sorry it's taken us so long to review it!

I'm honestly a bit torn on this proposal:

I've previously implemented something just like this at work for very similar reasons, so I understand where you're coming from. It did force developers into the habit of testing/checking their work before committing and pushing which was good. However, it also annoyed them to the point where they started using the --no-verify flag to avoid these automated checks. This was especially true for developers who tend to commit often and rebase/squash later.

Personally I think it would be better if we updated CONTRIBUTING.md and/or README.md to be more clear about which commands to run and when. And if the developer forgets to do this, CI will catch the issues for them, preventing the merge. (Avoiding big red errors in PRs can be enough motivation to encourage testing before pushing.)

That's just my $0.02 though - others may feel differently. I'm definitely open to hearing your thoughts and being persuaded 😛

Again, I do appreciate you taking the time to propose this, and for your patience in getting feedback.

[NigelGreenway]

Hi @colinodell,

You are totally right, this PR is very much one that has a a strong pro and con. I personally feel it's the best thing to have things automated with what we as engineers have to do on an hourly basis. We have implemented this in our projects at work to mirror the projects that are written in NodeJS (using Husky) and I must admit it has caught me a few times especially stopping those pushes where I've broken something without realising, or rushed a fix/feature and not ran the tests or linting locally.

The con, as you indicate, is that the engineer will of course use --no-verify - which I've done before but only when using a WIP branch of some sort - which will bypass this feature with the possibly argument that they shouldn't have to run tests/lint/whatever else locally as that is what your CI pipeline is for, my counter argument to that is places where there are large agencies or in house teams using micro services where everyone is pushing and your build is queued and when you are aware of the build you could have found that breakage 20-30 minutes previous.

That's my $0.02 😜

There could be a solution where by on post-install in composer a script could be added to ask if the maintainer of the project using the skeleton would like it installed or not?

As for the time on the reply, honestly don't worry. Open source is great, but I appreciate people have a life beyond coding... don't we? 😉

[NigelGreenway]

Just wanted to check in to see if I should fix the conflicts on this PR? 😃