build: add pre-commit package for linting #823

KevinEady · 2020-10-16T10:02:20Z

This extends #819 with a pre-commit hook to run the lint task prior to making a git commit.

For example, after I installed the package and tried to make a commit via:

git commit -m "build: add pre-commit package for linting"

I received this error, because the linting failed:

... output from lint task giving needed code changes ...
+                              0,
+                              1,
+                              testData,
+                              std::function<decltype(AcquireFinalizerCallback)>(
+                                  AcquireFinalizerCallback),
+                              testData);
 
   Object result = Object::New(env);
   result["createThread"] = Function::New( env, CreateThread, "createThread", testData);

      ERROR: please run "npm run lint:fix" to format changes in your commit
pre-commit: 
pre-commit: We've failed to pass the specified git pre-commit hooks as the `lint`
pre-commit: hook returned an exit code (1). If you're feeling adventurous you can
pre-commit: skip the git pre-commit hooks by adding the following flags to your commit:
pre-commit: 
pre-commit:   git commit -n (or --no-verify)
pre-commit: 
pre-commit: This is ill-advised since the commit is broken.
pre-commit:

I was able to forcefully make the commit by adding the -n flag as mentioned in the error.

@legendecas we discussed that the linter runs on the CI because of cb0764a#diff-6ac3f79fc25d95cd1e3d51da53a4b21b939437392578a35ae8cd6d5366ca5485R57 but i find it odd that your PR #819 succeeded, but my local lint task is failing...?

mhdawson · 2020-10-23T15:14:09Z

This should land after: #819

mhdawson

LGTM

legendecas · 2020-11-01T03:08:06Z

we discussed that the linter runs on the CI because of cb0764a#diff-6ac3f79fc25d95cd1e3d51da53a4b21b939437392578a35ae8cd6d5366ca5485R57 but i find it odd that your PR #819 succeeded, but my local lint task is failing...?

The checks run against the local master ref. So if the local master ref is outdated, the lint will probably fail for those seemingly unrelated lines. In the CI the comparison branch will always be updated, so it will always require the PR to be rebased on the latest main branch commit so that the checks will not complain about unrelated lines.

legendecas · 2020-11-10T02:48:28Z

@mhdawson it seems like #819 is not landed before this.

* build: add incremental clang-format checks * build: add pre-commit package for linting

legendecas and others added 2 commits Oct 7, 2020

build: add incremental clang-format checks

cb0764a

build: add pre-commit package for linting

d04fb8b

mhdawson approved these changes Oct 23, 2020

View changes

mhdawson merged commit 2c02d31 into nodejs:master Nov 9, 2020
1 check passed

Superlokkus added a commit to Superlokkus/node-addon-api that referenced this issue Nov 20, 2020

build: add pre-commit package for linting (nodejs#823)

71fe621

* build: add incremental clang-format checks * build: add pre-commit package for linting

Mar	APR	May
	05
2021	2022	2023

nodejs / node-addon-api Public

build: add pre-commit package for linting #823

build: add pre-commit package for linting #823

KevinEady commented Oct 16, 2020

mhdawson commented Oct 23, 2020

mhdawson left a comment

legendecas commented Nov 1, 2020

legendecas commented Nov 10, 2020

nodejs / node-addon-api Public

build: add pre-commit package for linting #823

build: add pre-commit package for linting #823

Conversation

KevinEady commented Oct 16, 2020

mhdawson commented Oct 23, 2020

mhdawson left a comment

legendecas commented Nov 1, 2020

legendecas commented Nov 10, 2020