ghooks configuration #81

ta2edchimp · 2016-04-13T17:09:56Z

This PR introduces the ability to configure ghooks more detailed (as originally proposed here).

In package.json:

  "config": {
    "ghooks": {
        "randomConfig": ["randomValue"],
        "hooks": {
          "pre-commit": "echo \"some pre-commit hook\"
        }
    }

Replaces the old way (still working, but considered deprecated):

  "config": {
    "ghooks": {
        "pre-commit": "echo \"some pre-commit hook\""
    },
    "ghooks-randomConfig": ["introduced for backwards compatibility with hooks configuration"]
  }

The idea behind this PR is, for example to prepare ghooks to be able to optionally exit the hooks with a non-zero code (an error) if ghooks could not be loaded successfully. Therefore, I consider this PR as work in progress, until that particular feature can be (safely, this time) enabled again.

So in order to re-implement an »exit with error« functionality, I'd like to know what way to approach this would be the most adequate:

upon install, check the config whether to »exit with error« and write the hooks' script accordingly
try to obtain the responsible package.json and read the config, not exiting with an error if nothing explicitly states so.

When going route 1, I'd suggest making the install script "re-runnable", by adding it as bin.ghooks to the package.json.

@gtramontina @kentcdodds I'd like to hear from you what your thoughts are about this proposal (if/when you got time for this — no need to hurry).

Edit: If this gets merged eventually, I'd propose we call this a breaking change to trigger a major version bump?

ta2edchimp · 2016-04-13T17:29:47Z

Meh. CI builds seem to fail due to a bug in istanbul 😞

kentcdodds · 2016-04-13T21:03:03Z

test/config.test.js

+    expect(getConfig()).to.deep.equal(expectedConfig)
+  })
+
+  it('reads custom configuration and hooks (overriding the deprecated way)', () => {


great coverage!

kentcdodds · 2016-04-13T21:23:37Z

This looks awesome!

If this gets merged eventually, I'd propose we call this a breaking change to trigger a major version bump?

There's technically nothing breaking about this right? Unless I misunderstand what's going on here, it looks like we're just adding the ability to configure things. Seems fine to me. When we remove the ability to do the old stuff then we can version bump. But it looks like we've got backward compat right now.

ta2edchimp · 2016-04-13T21:46:00Z

Yes, technically there shouldn't be anything breaking; I guess I only wanted to "feel" safe after "exit with error-gate" 😜

Atm I have the following steps on my todo list for the upcoming days:

Have a look into the code coverage issue.
Re-/Implement the "exit with error" option.
Update the ReadMe accordingly.

Please drop me a line here, if you'd prefer to put the (re-)implementation of "exit with error" into a separate PR (on the other hand, there would be no need for these changes, when there's nothing to configure).

... and thanks for taking the time to review this!

kentcdodds · 2016-04-15T05:30:12Z

Please drop me a line here, if you'd prefer to put the (re-)implementation of "exit with error" into a separate PR (on the other hand, there would be no need for these changes, when there's nothing to configure).

I'm fine doing this in the same PR as long as it's backward compatible 👍

ta2edchimp · 2016-04-18T20:16:26Z

lib/hook.template.raw

+  // it gets substituted on ghooks' setup
+  // {{generated_config_start}}
+  return global.__testonly_ghooksConfig__ || {}
+  // {{generated_config_end}}


global.__testonly_ghooksConfig__ is atm required to thoroughly test the optional behavior to exit with an error.

The whole block from {{generated_config_start}} to {{generated_config_end}} (including their comments) gets replaced when rendering the current config into the hook.template.

As a side note:
The hooks also get rendered into the template. I was unsure whether to ditch them, but maybe we can make use of them at a later point.

gtramontina · 2016-05-27T16:54:25Z

Hey everyone, really sorry for the silence… This looks great!

One other option, that just occurred me, could be simply having another config entry. Just like we have config.ghooks, I'm thinking we could also support config.ghooks-settings (or config.ghooks-options, or…) 💭 ?

When going route 1, I'd suggest making the install script "re-runnable", by adding it as bin.ghooks to the package.json.

If I correctly got what you're saying, I believe we can already re-run it with npm run ghooks install.

kentcdodds · 2016-05-27T16:58:32Z

I'd be fine having a separate config property called ghooksSettings or whatever. I forget why we need settings for ghooks now though. It's been a while :-)

gtramontina · 2016-05-27T17:00:41Z

I thought an example would be useful… 😛

{
  "config": {
    "ghooks": {
      "pre-commit": "",
      …
    },
    "ghooks-settings": {
      "exitWithError": true
    }
  }
}

_{gonna sleep now, it's freaking 3am here o_O}

ta2edchimp · 2016-05-27T17:39:52Z

I forget why we need settings for ghooks now though. It's been a while :-)

It was mostly because of »exit with error-gate« and #28 (Specify project root manually).

Some place was concluded that it would be nice to eventually have

"config": {
    "ghooks": {
        "randomConfig": ["randomValue"],
        "hooks": {
          "pre-commit": "echo \"some pre-commit hook\"
        }
    }

and to allow config.ghooks-randomConfig (whilst marked as deprecated) too — for reasons of backwards compatibility with config.ghooks[ hookname ].

I should have some more time in the upcoming days, I'll at least update this PR / resolve the conflicts, so we'd have a clean status we'd be talking about ...

codecov-io · 2016-05-31T08:51:12Z

Current coverage is 100%

Merging #81 into master will decrease coverage by n/a%

@@            master     #81   diff @@
======================================
  Files            4       5     +1   
  Lines          114     166    +52   
  Methods          0       7     +7   
  Branches         4       6     +2   
======================================
+ Hits           114     166    +52   
  Misses           0       0          
  Partials         0       0

Powered by Codecov. Last updated by 73262f4...db08d89

Allow ghooks to be configured more detailed via config.ghooks.[option] in package.json. Hooks are now to be configured via the new config.ghooks.hooks config-object in package.json. (Still valid, but considered deprecated: config.ghooks.[hookname]).

Required on node 0.10

ta2edchimp · 2016-05-31T09:13:27Z

Jesus Christ, that GitCop tried hard to get me arrested for being too verbose . . . 😞

gtramontina · 2016-05-31T11:12:40Z

😆 I guess I should tune it down a little…

gtramontina · 2016-06-07T23:23:35Z

What do you think about warning the developer if they are using the deprecated (hopefully soon-to-be removed) way?

ta2edchimp · 2016-06-08T08:41:54Z

Absolutely! I'm just unsure as where to output the warning (as in, on every invokation of a hook, only on the hooks' installation/setup, ...).

kentcdodds · 2016-06-24T19:19:27Z

Re: GitCop, it seemed like a good idea at the time, but.... I don't use it anymore. Just too heavy handed and not flexible.

gtramontina · 2017-01-04T22:38:35Z

Closing this one as there will be no future development in ghooks. Thank you for your efforts, @ta2edchimp !

kentcdodds reviewed Apr 13, 2016
View reviewed changes

ta2edchimp mentioned this pull request Apr 14, 2016

chore(coverage): Replace istanbul with nyc #83

Merged

ta2edchimp force-pushed the feat/pkg-config branch from 2f5c096 to d128feb Compare April 14, 2016 14:59

ta2edchimp changed the title ~~WIP: ghooks configuration~~ ghooks configuration Apr 18, 2016

ta2edchimp reviewed Apr 18, 2016
View reviewed changes

ta2edchimp force-pushed the feat/pkg-config branch from 51c717e to 9a1dc4f Compare May 31, 2016 08:48

ta2edchimp force-pushed the feat/pkg-config branch 2 times, most recently from bf49af4 to 32697d0 Compare May 31, 2016 08:57

ta2edchimp added 5 commits May 31, 2016 11:01

refactor(config): Fix tests to meet coverage threshold

16747bf

chore(tests): Add babel-polyfill to tests and coverage scripts

603a12a

Required on node 0.10

feat(hook): Exit with error, if configured

85f41fd

docs(readme): Add description on configuration

0b374f9

ta2edchimp force-pushed the feat/pkg-config branch from 32697d0 to 0b374f9 Compare May 31, 2016 09:04

ta2edchimp mentioned this pull request Jun 4, 2016

execa (dependency) should probably be updated #103

Closed

kentcdodds mentioned this pull request Jun 24, 2016

Specify project root manually #28

Closed

gtramontina closed this Jan 4, 2017

ta2edchimp mentioned this pull request Jun 19, 2017

Hook, line and husky #209

Closed

gtramontina mentioned this pull request Apr 29, 2018

feat(install): support custom node bin path in shebang #225

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ghooks configuration #81

ghooks configuration #81

ta2edchimp commented Apr 13, 2016

ta2edchimp commented Apr 13, 2016

kentcdodds Apr 13, 2016

kentcdodds commented Apr 13, 2016

ta2edchimp commented Apr 13, 2016 •

edited

Loading

kentcdodds commented Apr 15, 2016

ta2edchimp Apr 18, 2016

gtramontina commented May 27, 2016

kentcdodds commented May 27, 2016

gtramontina commented May 27, 2016

ta2edchimp commented May 27, 2016

codecov-io commented May 31, 2016 •

edited

Loading

ta2edchimp commented May 31, 2016

gtramontina commented May 31, 2016

gtramontina commented Jun 7, 2016

ta2edchimp commented Jun 8, 2016

kentcdodds commented Jun 24, 2016

gtramontina commented Jan 4, 2017

ghooks configuration #81

ghooks configuration #81

Conversation

ta2edchimp commented Apr 13, 2016

ta2edchimp commented Apr 13, 2016

kentcdodds Apr 13, 2016

Choose a reason for hiding this comment

kentcdodds commented Apr 13, 2016

ta2edchimp commented Apr 13, 2016 • edited Loading

kentcdodds commented Apr 15, 2016

ta2edchimp Apr 18, 2016

Choose a reason for hiding this comment

gtramontina commented May 27, 2016

kentcdodds commented May 27, 2016

gtramontina commented May 27, 2016

ta2edchimp commented May 27, 2016

codecov-io commented May 31, 2016 • edited Loading

Current coverage is 100%

ta2edchimp commented May 31, 2016

gtramontina commented May 31, 2016

gtramontina commented Jun 7, 2016

ta2edchimp commented Jun 8, 2016

kentcdodds commented Jun 24, 2016

gtramontina commented Jan 4, 2017

ta2edchimp commented Apr 13, 2016 •

edited

Loading

codecov-io commented May 31, 2016 •

edited

Loading