search

wecodemore / grunt-githooks

A Grunt plugin to help bind Grunt tasks to Git hooks
https://npmjs.org/package/grunt-githooks
MIT License
317 stars 24 forks source link

Branch safety Issue #21 #33

Closed CamdenSegal closed 7 years ago

CamdenSegal commented 10 years ago

This is a big change, it changes the core of how the hooks call grunt tasks, from directly calling them to calling the githook task with the current hook as an argument. While I think this is preferable I understand it is a big change and I'm happy to hear any suggestions. I'm also happy to publish this change as a separate fork if you think it changes too much.

franz-josef-kaiser commented 10 years ago

First, thanks for your PR. Would you mind opening an issue to discuss it?

Starting point would be to explain what you are aiming for. As far as I understood it from reading the plain diff (no tests?), you are introducing a new option named hook? And that hook is there to attach grunt tasks? Because as it currently stands, the plugin isn't meant to be limited to Grunt tasks only. I personally use it for PHP mostly.

CamdenSegal commented 10 years ago

There is an issue, but it is currently closed (#21).

I did modify tests but overall the end functionality is very similar just how it achieves it is very different so most of the tests still apply.

The modification is that the templates now check for the Gruntfile before running and runs a generic task such as grunt githook::precommit which the runs all the tasks associated with that hook for every target. This allows the hook to only run the tasks currently assigned to that hook in the Gruntfile no matter the branch and without the necessity of rebinding.

I would argue that you should setup separate grunt tasks for the non-grunt actions you want to do so it can hook in properly, but again if you feel like this change isn't something you want for this plugin I can publish it as a separate fork.

franz-josef-kaiser commented 10 years ago

Ah, ok - I remember now.

I will just need some time to look into this PR and the consequences/changes it adds. Especially the non-grunt-actions as grunt tasks.

Oh and you already have published a fork ;)

franz-josef-kaiser commented 10 years ago

Just read the email from this again - reopened #21

webuniverseio commented 8 years ago

:+1: Also would be nice to check that task exists. I might setup hook to run task in one branch, but then when I switch to another, task might not exists there. Thanks.

franz-josef-kaiser commented 8 years ago

@szarouski We already had that discussion in some issue. Problem is, that in some branches you might not even have the grunt hooks package available or some Git hooks aren't present there. If you have a solution, I suggest to open an issue for discussion before filing a PR. In case you want this PR to go in, it would need an update – feel free to jump in.

franz-josef-kaiser commented 7 years ago

Closing as there was no more progress or discussion. Anyway, thanks for taking the time! :)