Skip to content

Conversation

@KevinEady
Copy link
Contributor

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
Copy link
Member

This should land after: #819

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@legendecas
Copy link
Member

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.

@mhdawson mhdawson merged commit 2c02d31 into nodejs:master Nov 9, 2020
@legendecas
Copy link
Member

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

Superlokkus pushed a commit to Superlokkus/node-addon-api that referenced this pull request Nov 20, 2020
* build: add incremental clang-format checks
* build: add pre-commit package for linting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants