Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide option to remove old hooks #12

Open
mokkabonna opened this issue Nov 13, 2013 · 8 comments
Open

Provide option to remove old hooks #12

mokkabonna opened this issue Nov 13, 2013 · 8 comments

Comments

@mokkabonna
Copy link

Either clean whole directory and install new hooks. Or more unobtrusive, check all hooks if they are previously installed by this grunt plugin, and if it is removed from gruntfile now then remove it form the hooks directory.

Not a huge need for it as you could also combine this with a clean task, but not so good if you have other non grunt hooks installed.

Consider it, anyway thanks for this plugin. :)

@romaricpascal
Copy link
Contributor

Cleaning up what grunt-githooks created and removing legacy hooks would be two nice additions to grunt-githooks indeed.

I don't think the plugin should remove the files, though, as the plugin inserts/appends rather than replaces. Removing the snippet sounds enough to me.

Detecting if githooks created the file and remove it in this case could be a clever step up from this. The plugin would need to make sure the user has not added more code to the hook, which might prove a bit tricky.

Not sure when I'll be able to fit these feature in, but they're definitely something I'd like to add. Thanks for the input :)

@RSeidelsohn
Copy link

Dear rhumaric,

thanks for this great grunt plugin. Yes, I second this idea - it would be nice if we could set up something like
{ 'remove': 'pre-commit' } and that subtask would then simply delete everything in-between the defined start and end comments (including the comments themselves).
I've just set up a default githooks task that sets a pre-commit-hook and a post-merge-hook, but then it turned out some devs in our team prefer to only run an update-hook (on 'git push'), so it would be great if I just could set up subtasks deleting the hooks again without having to manually edit the git hook files.

Cheers,
Roman.

@danyshaanan
Copy link

After adding a hook that caused problems, I've removed it with grunt-contrib-clean. Yet, as said above, it would be very nice to enabled removing hooks by grunt-githooks.

although it wouldn't be backwards compatible, I'm imagining something like this:

Every hook added could be wrapped with:

#grunt-githooks start
#The actual hook code
#grunt-githooks end

This will make clearing a file from grunt-githooks hooks, very easy, however I have no idea how reasonable such implementation would be in the current code. Might check it out when I'll have a little more time :]

@franz-josef-kaiser
Copy link
Member

@Augenfeind @danyshaanan The problem that I'd see with hook "blocks" added on special mark up is the Regex itself. Not the creation, but the fact that there are a zillion valid programming languages out there and that the comment style could easily interfere with them.

Example: While # is a valid comment in Bash or PHP, it's not in JavaScript. So when adding such a block in JS, we'd have to prefix that with a double slash // which then would be a rule on top of a rule.

In short, this won't work this way. Or at least it wouldn't be reliable or even testable and we would need to greatly close our scope on tested languages.

What I'd suggest is that we simply include a workflow recommendation and add it to the docs. When you look at our GitPHPHooks package, then you will see that we simply use the hookfile as autoloader script. Example:

#!/usr/bin/env php
<?php

include 'vendor/wcm/git-php-hooks/GitHooksLoader.php';
new \GitHooksLoader( __FILE__, 'git-hooks' );

The actual scripts from our GitPHPHooksLibrary project get loaded from there. So the only thing that needs to be maintained is the library or in case there are more libraries, just the autoloader to include them. In the later case we could think about adding an option to either delete and recreate or append GruntHooks.

Opinions? PRs for our README?

@danyshaanan
Copy link

Side-note: I believe that this problem should have been solved within git, by defining hooks using a wildcard: /^pre-commit(-.*)?$/, but never mind that.

I don't see any problem with the special escaping, as, if I understand correctly, "options.startMarker " and "options.endMarker" are always used, but I do see a problem with the fact that the user can change the markers during work, or even choose markers that are empty or likely repeatable.

A different approach would require a user who uses a template of his own to define the code escaping string, and make the marker unique enough. ("GRUNT-GITHOOKS START" or even "wecodemore/grunt-githooks start"), but that would require a breaking change or handling two sets of options.

Therefore, sadly, I'd have to agree that a workflow recommendation might be the best option, other than a massive change of spec.

@danyshaanan
Copy link

Another approach would be to write to the main hooks only something like this:

if (hookname-grunt-githooks exists) execute hookname-grunt-githooks

(which could be easy to add in the script language an existing hook is written in)
and place everything else inside that easily removable file which is specific to this module

@franz-josef-kaiser
Copy link
Member

The later option pretty much is the same as the workflow recommendation, right?

@danyshaanan
Copy link

Yeah, (you lost me there for a minute with 'php'). The question is, how can this be automated, so that users could still be able to define which tasks to run, without creating or copying the code themselves.

Maybe options.costumHookFileSuffix could tell the suffix of the name of the file to be imported from the main hook (if exists), and in which to put the specific code:

grunt.initConfig({
  githooks: {
    options: {
              costumHookFileSuffix: '-grunt-githooks'
    },
    all: {
      'pre-commit': 'jshint',
      'pre-push': 'anotherGruntTask'
    }
  },
})

This will:

  • Add a conditional include to pre-commit for:
  • pre-commit-grunt-githooks - which will include the actual template with jshint
  • Add a conditional include to pre-push for:
  • pre-push-grunt-githooks - which will include the actual template with anotherGruntTask

This will allow an easy and safe cleanup using grunt-contrib-clean of unwanted hooks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants