Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix tab indent #938

Merged
merged 2 commits into from
Mar 22, 2021
Merged

Fix tab indent #938

merged 2 commits into from
Mar 22, 2021

Conversation

tniessen
Copy link
Member

There is an argument to be made for using tabs, but the mixture of tabs and spaces causes visual inconsistencies in the docs and source code.

@tniessen
Copy link
Member Author

The linter was complaining (see commit ed55eec) about things that are not strictly related to this change. In other words, the linter should not fail as far as I can tell. Anyway, shouldn't be too difficult to make the linter happy, right? However, it turned into something extremely inconvenient for someone who doesn't know the tools involved here. Maybe I misunderstood the instructions, but I only got it to accept the patch by manually invoking git-clang-format and then disabling the precommit hook. This seems... wrong. (cc @legendecas)


Local git log after rebasing:

b29073e (HEAD -> fix-tab-indent) src: fix tab indent
b173956 doc: fix tab indent
87b7aae (upstream/main, main) doc: warn about SuppressDestruct() (#926)

npm run lint:

$ npm run lint

> [email protected] lint
> node tools/clang-format.js

diff --git a/napi-inl.h b/napi-inl.h
index 87e65d0..1788078 100644
--- a/napi-inl.h
+++ b/napi-inl.h
@@ -4150,8 +4150,7 @@ inline AsyncContext::AsyncContext(napi_env env, const char* resource_name)
 inline AsyncContext::AsyncContext(napi_env env,
                                   const char* resource_name,
                                   const Object& resource)
-  : _env(env),
-    _context(nullptr) {
+    : _env(env), _context(nullptr) {
   napi_value resource_id;
   napi_status status = napi_create_string_utf8(
       _env, resource_name, NAPI_AUTO_LENGTH, &resource_id);

      ERROR: please run "npm run lint:fix" to format changes in your commit
        Note that when running the command locally, please keep your local
        main branch and working branch up to date with nodejs/node-addon-api
        to exclude un-related complains.
        Or you can run "env CLANG_FORMAT_START=upstream/main "npm run lint:fix"".

As suggested, npm run lint:fix (branches are up-to-date):

$ npm run lint:fix

> [email protected] lint:fix
> git-clang-format '*.h', '*.cc'

no modified files to format

As suggested, env CLANG_FORMAT_START=upstream/main "npm run lint:fix":

$ env CLANG_FORMAT_START=upstream/main "npm run lint:fix"
env: ‘npm run lint:fix’: No such file or directory

Okay, env CLANG_FORMAT_START=upstream/main npm run lint:fix without quotes instead:

$ env CLANG_FORMAT_START=upstream/main npm run lint:fix

> [email protected] lint:fix
> git-clang-format '*.h', '*.cc'

no modified files to format

Maybe it only checks unstaged changes?

$ git reset HEAD^
Unstaged changes after reset:
M       napi-inl.h
$ env CLANG_FORMAT_START=upstream/main npm run lint:fix

> [email protected] lint:fix
> git-clang-format '*.h', '*.cc'

no modified files to format

Okay, let's invoke it manually then.

$ ./node_modules/.bin/git-clang-format main -- napi-inl.h
changed files:
    napi-inl.h
$ git diff
diff --git a/napi-inl.h b/napi-inl.h
index 87e65d0..1788078 100644
--- a/napi-inl.h
+++ b/napi-inl.h
@@ -4150,8 +4150,7 @@ inline AsyncContext::AsyncContext(napi_env env, const char* resource_name)
 inline AsyncContext::AsyncContext(napi_env env,
                                   const char* resource_name,
                                   const Object& resource)
-  : _env(env),
-    _context(nullptr) {
+    : _env(env), _context(nullptr) {
   napi_value resource_id;
   napi_status status = napi_create_string_utf8(
       _env, resource_name, NAPI_AUTO_LENGTH, &resource_id)

Well, I guess that looks somewhat okay. So I should be able to commit then?

$ git stage napi-inl.h && git commit --amend
diff --git a/napi-inl.h b/napi-inl.h
index 87e65d0..1788078 100644
--- a/napi-inl.h
+++ b/napi-inl.h
@@ -4150,8 +4150,7 @@ inline AsyncContext::AsyncContext(napi_env env, const char* resource_name)
 inline AsyncContext::AsyncContext(napi_env env,
                                   const char* resource_name,
                                   const Object& resource)
-  : _env(env),
-    _context(nullptr) {
+    : _env(env), _context(nullptr) {
   napi_value resource_id;
   napi_status status = napi_create_string_utf8(
       _env, resource_name, NAPI_AUTO_LENGTH, &resource_id);

      ERROR: please run "npm run lint:fix" to format changes in your commit
        Note that when running the command locally, please keep your local
        main branch and working branch up to date with nodejs/node-addon-api
        to exclude un-related complains.
        Or you can run "env CLANG_FORMAT_START=upstream/main "npm run lint:fix"".

Nope, still cannot commit. The precommit hook appears to be complaining about the patch that git-clang-format applied. Running the suggested command again also does not work.

$ npm run lint:fix

> [email protected] lint:fix
> git-clang-format '*.h', '*.cc'

no modified files to format

A second manual run also does nothing:

$ ./node_modules/.bin/git-clang-format main -- napi-inl.h
clang-format did not modify any files

The only solution seems to be to disable the precommit hook:

$ git commit --amend --no-verify
[fix-tab-indent 653293b] src: fix tab indent
 Date: Wed Mar 17 12:47:06 2021 +0100
 1 file changed, 2 insertions(+), 3 deletions(-)

$ npm run lint

> [email protected] lint
> node tools/clang-format.js

Finally no error.

Copy link
Member

@NickNaso NickNaso left a comment

Choose a reason for hiding this comment

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

LGTM

@NickNaso NickNaso added doc and removed doc labels Mar 18, 2021
@legendecas
Copy link
Member

@tniessen thanks for report, I'll try to dig into the problem here.

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

@mhdawson mhdawson merged commit 7319a0d into nodejs:main Mar 22, 2021
deepakrkris pushed a commit to deepakrkris/node-addon-api that referenced this pull request Sep 23, 2021
* doc: fix tab indent
@tniessen tniessen deleted the fix-tab-indent branch October 7, 2021 16:52
deepakrkris pushed a commit to deepakrkris/node-addon-api that referenced this pull request Oct 15, 2021
* doc: fix tab indent
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.

5 participants