Skip to content

Fix tab indent#938

Merged
mhdawson merged 2 commits intonodejs:mainfrom
tniessen:fix-tab-indent
Mar 22, 2021
Merged

Fix tab indent#938
mhdawson merged 2 commits intonodejs:mainfrom
tniessen:fix-tab-indent

Conversation

@tniessen
Copy link
Copy Markdown
Member

@tniessen tniessen commented Mar 17, 2021

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
Copy Markdown
Member Author

tniessen commented Mar 18, 2021

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

> node-addon-api@3.1.0 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

> node-addon-api@3.1.0 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

> node-addon-api@3.1.0 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

> node-addon-api@3.1.0 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

> node-addon-api@3.1.0 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

> node-addon-api@3.1.0 lint
> node tools/clang-format.js

Finally no error.

NickNaso
NickNaso approved these changes Mar 18, 2021
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Member

legendecas commented Mar 19, 2021

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

@legendecas legendecas mentioned this pull request Mar 19, 2021
Merged
mhdawson
mhdawson approved these changes Mar 22, 2021
View reviewed changes
Copy link
Copy Markdown
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
@tniessen
Fix tab indent (nodejs#938)
e320666 
* 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
@tniessen
Fix tab indent (nodejs#938)
4e3a2f9 
* doc: fix tab indent
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@NickNaso NickNaso NickNaso approved these changes

@gabrielschulhof gabrielschulhof gabrielschulhof approved these changes

@mhdawson mhdawson mhdawson approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@tniessen @legendecas @NickNaso @gabrielschulhof @mhdawson