Add pre-commit hook and setup script #129

2019-05-14T10:43:54Z

raucao commented

2019-05-14 10:43:54 +00:00

(Migrated from github.com)

Runs the appropriate linter on staged files before committing.

Can be easily installed in the local repo by running npm run setup.

Runs the appropriate linter on staged files before committing. Can be easily installed in the local repo by running `npm run setup`.

bumi (Migrated from github.com) reviewed 2019-05-16 09:25:55 +00:00

README.md

bumi (Migrated from github.com) commented

2019-05-16 09:25:34 +00:00

    $ npm run setup-git-hooks

```suggestion $ npm run setup-git-hooks ```

package.json

bumi (Migrated from github.com) commented

2019-05-16 09:25:10 +00:00

    "setup-git-hooks": "sh scripts/git-hooks/install"

I find this name a bit confusing as it sounds it has something todo with the setup to install/run the code. We have bootstrap, reset, install-all - so I'd by a bit more specific here as so far this setup task is only installing the git hooks.

```suggestion "setup-git-hooks": "sh scripts/git-hooks/install" ``` I find this name a bit confusing as it sounds it has something todo with the setup to install/run the code. We have `bootstrap`, `reset`, `install-all` - so I'd by a bit more specific here as so far this setup task is only installing the git hooks.

raucao (Migrated from github.com) reviewed 2019-05-22 08:29:46 +00:00

package.json

raucao (Migrated from github.com) commented

2019-05-22 08:29:46 +00:00

This setup is meant to include anything that is necessary, other than npm install, to set up the repo itself. In fact, I'd probably make this a postinstall script, so one doesn't even have to run it manually.

This setup is meant to include anything that is necessary, other than `npm install`, to set up the repo itself. In fact, I'd probably make this a `postinstall` script, so one doesn't even have to run it manually.

raucao (Migrated from github.com) reviewed 2019-05-22 08:39:37 +00:00

package.json

raucao (Migrated from github.com) commented

2019-05-22 08:39:37 +00:00

Moved the entire thing to postinstall now.

Moved the entire thing to `postinstall` now.

raucao commented

2019-05-22 08:40:49 +00:00

(Migrated from github.com)

There's a weird issue with npm, in that when running a postinstall script it re-validates the entire npm dependency stack. I don't know why that is.

There's a weird issue with npm, in that when running a `postinstall` script it re-validates the entire npm dependency stack. I don't know why that is.

raucao commented

2019-06-02 10:22:19 +00:00

(Migrated from github.com)

@bumi Can we merge this now?

bumi commented

2019-06-08 15:08:53 +00:00

(Migrated from github.com)

Isn't postinstall also executed when this is installed as a dependency? e.g. in kredits-web?

if it only runs when running npm install I am ok with merging it. though as much as I need this am not a fan of postinstall scripts doing such modifications.
when we tried to run the app installations in a post-install script it was also only causing problems (don't remember the details though).

Isn't `postinstall` also executed when this is installed as a dependency? e.g. in kredits-web? if it only runs when running `npm install` I am ok with merging it. though as much as I need this am not a fan of postinstall scripts doing such modifications. when we tried to run the app installations in a post-install script it was also only causing problems (don't remember the details though).

raucao commented

2019-06-09 11:53:20 +00:00

(Migrated from github.com)

Renamed it to your suggestion.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: kredits/contracts#129