Skip to content
This repository has been archived by the owner on Nov 27, 2019. It is now read-only.

chore: Don't check readme pre push but check commit message #599

Closed

Conversation

freaktechnik
Copy link
Contributor

This runs the changelog-lint script at the git "commit-msg" hook, like the web-ext setup does. I've also tested it on Windows, where it works, except with GitHub for Windows, which probably doesn't support git hooks, though I suspect push would fail there too.

Further it removes the documentation spellcheck from pre-push. While doing that, I was wondering, if ti was worth adding --cache to the pre-push eslint, though I think there's not enough push frequency and it's not that slow to warrant that.

This fixes #594

@kumar303
Copy link
Contributor

Honestly, I think we can remove the eslint check from the pre-push hook altogether. The main reason I wanted to get the changelog-lint as a commit hook is because once you push a bad commit message, the PR will fail indefinitely and this is hard to fix if you aren't familiar with rebasing.

Copy link
Contributor

@kumar303 kumar303 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This didn't work for me when I tried committing a message that was formatted incorrectly. I did not receive an error.

I think it's because npm run changelog-lint only looks at previously commits by default. The way we do this on web-ext is with a custom script that pipes the temporary commit message file into the lint checker: https://github.com/mozilla/web-ext/blob/master/.githooks/commit-msg/check-for-changelog-lint#L7

Is there an easy way to do that with husky?

@freaktechnik
Copy link
Contributor Author

@kumar303 just tested this version in a windows shell with git and it worked fine, though I still had an error when using the GitHub client.

@kumar303
Copy link
Contributor

Huh, this is confusing. On Mac OS X (tried in Node 6.3.0 and 4.3.0), I see this:

$ git commit -a -m 'nope'

> [email protected] commitmsg /Users/kumar/dev/jpm
> cat "$GIT_PARAMS" | conventional-changelog-lint

/Users/kumar/dev/jpm/node_modules/conventional-changelog-lint/distribution/cli.js:6
let main = function () {
^^^
SyntaxError: Unexpected strict mode reserved word
    at exports.runInThisContext (vm.js:73:16)
    at Module._compile (module.js:443:25)
    at Object.Module._extensions..js (module.js:478:10)
    at Module.load (module.js:355:32)
    at Function.Module._load (module.js:310:12)
    at Function.Module.runMain (module.js:501:10)
    at startup (node.js:129:16)
    at node.js:814:3

However, if I run this in my shell it works as expected:

$ cat .git/COMMIT_EDITMSG | conventional-changelog-lint
⧗   input: nope
✖   message may not be empty [subject-empty]
✖   type may not be empty [type-empty]
✖   type must be one of ["chore", "docs", "feat", "fix", "perf", "refactor", "style", "test"] [type-enum]
✖   found 3 problems, 0 warnings

I don't understand what black magic npm (or husky) might be doing to cause this. On a hunch, I tried applying this patch but it made no difference:

diff --git a/package.json b/package.json
index 0c9a56f..511c0c6 100644
--- a/package.json
+++ b/package.json
@@ -19,9 +19,10 @@
     "lint": "eslint bin/jpm lib test",
     "test": "node ./test/run.js",
     "prepush": "npm run lint",
-    "commitmsg": "echo \"params: $GIT_PARAMS\"",
+    "commitmsg": "cat \"$GIT_PARAMS\" | npm run changelog-lint-from-stdin --silent",
     "changelog": "conventional-changelog -p angular -u",
     "changelog-lint": "conventional-changelog-lint --from master",
+    "changelog-lint-from-stdin": "conventional-changelog-lint",
     "get-unbranded-firefox": "get-firefox -ecb unbranded-release"
   },
   "homepage": "https://github.com/mozilla-jetpack/jpm",

Any ideas on what could be wrong? I take it that you do not see this on your system?

@freaktechnik
Copy link
Contributor Author

freaktechnik commented Oct 17, 2016

The important bit is updating the husky version, too and then regenerating the git hooks, since the current version the package.json specifies does not have $GIT_PARAMS in the hook script template yet.

Edit: I wonder if this is some nvm oddness or something?

@kumar303
Copy link
Contributor

I removed my entire node_modules and re-installed a couple times (when trying it each Node version) but that didn't seem to help

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

Successfully merging this pull request may close these issues.

Run changelog lint pre-push?
2 participants