Skip to content

create tools/eslint-format#1080

Closed
rubiagatra wants to merge 2 commits intonodejs:mainfrom
rubiagatra:lint/1076-follow-up-js-linter
Closed

create tools/eslint-format#1080
rubiagatra wants to merge 2 commits intonodejs:mainfrom
rubiagatra:lint/1076-follow-up-js-linter

Conversation

@rubiagatra
Copy link
Copy Markdown
Contributor

@rubiagatra rubiagatra commented Sep 29, 2021 •
edited
Loading

Closes #1076

Tasks

Note

  • I changed CLANG_FORMAT_START -> FORMAT_START

Example

@mhdawson I created tools/eslint-format.js do you mind checking this one out? the example looks like this when pre-commit

Eslint error:
/Users/rubiagatra/github/node-addon-api/test/addon.js
  6:5  error  'a' is defined but never used  no-unused-vars

✖ 1 problem (1 error, 0 warnings)


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 FORMAT_START=upstream/main npm run lint:fix".
    Also fix JS files by yourself if necessary.
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:

@mhdawson
Copy link
Copy Markdown
Member

mhdawson commented Sep 30, 2021

@rubiagatra thanks, on my list to review, just have not gotten to it yet.

@mhdawson
Copy link
Copy Markdown
Member

mhdawson commented Oct 1, 2021

I tried this out today, but it does not seem to run the linter/find changed files in this case:

  1. Check out the main branch
  2. Apply the patch from this PR
  3. Modify test/version_management.js to introduce something that should be reported
  4. run npm run lint

I expect you probably tested when you were on a branch other than main, but I think we need this scenario to work as well as I think it's what will run in the CI.

If I modify test/version_management.cc' in a way that introduces something that should be reported and run npm run lint` I do get a report.

@rubiagatra can you take a look?

@mhdawson
Copy link
Copy Markdown
Member

mhdawson commented Oct 25, 2021

@rubiagatra just a reminder that I ran into a few issues and was hoping you could take a look.

@rubiagatra
Copy link
Copy Markdown
Contributor Author

rubiagatra commented Oct 26, 2021

thank you @mhdawson for the reminder, I will take a look today and tomorrow.

sorry, I am not in GitHub for a while

@rubiagatra
Copy link
Copy Markdown
Contributor Author

rubiagatra commented Oct 28, 2021

I am still continuing checking out this will give an update on friday

@rubiagatra
Copy link
Copy Markdown
Contributor Author

rubiagatra commented Nov 1, 2021

@mhdawson I follow your instruction.

  1. Check out the main branch
  2. Apply the patch from this PR
  3. Modify test/version_management.js to introduce something that should be reported
  4. run npm run lint

it will have outcome.

➜ node-addon-api (main) ✔ nvim test/version_management.js
➜ node-addon-api (main) ✗ npm run lint

> node-addon-api@4.2.0 lint
> node tools/eslint-format && node tools/clang-format

Eslint error:
/Users/rubiagatra/github/node-addon-api/test/version_management.js
  7:7  error  'a' is assigned a value but never used  no-unused-vars

✖ 1 problem (1 error, 0 warnings)


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 FORMAT_START=upstream/main npm run lint:fix".
    Also fix JS files by yourself if necessary.

maybe the linter of your code editor automatically fixes your update to the code?

I am just creating no-unused-vars error for example. For styling issue it will fix automatically with my code editor

@mhdawson
Copy link
Copy Markdown
Member

mhdawson commented Nov 8, 2021

I've not had a chance yet but will try to take a look sometime this week.

@mhdawson
Copy link
Copy Markdown
Member

mhdawson commented Nov 15, 2021

@rubiagatra I'm not sure what I was doing before, I went through the steps again and all seems to be working as expected sorry for the noise.

mhdawson
mhdawson approved these changes Nov 15, 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 pushed a commit that referenced this pull request Nov 15, 2021
@rubiagatra @mhdawson
test: create tools/eslint-format
706b199 
Improve JS linting and add option to fix linting errors

Fixes: #1076
PR-URL: #1080
Reviewed-By: Michael Dawson <midawson@redhat.com
@mhdawson
Copy link
Copy Markdown
Member

mhdawson commented Nov 15, 2021

Landed in 706b199

@mhdawson mhdawson closed this Nov 15, 2021
@rubiagatra
Copy link
Copy Markdown
Contributor Author

rubiagatra commented Nov 16, 2021

thanks! @mhdawson

kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this pull request Aug 24, 2022
@kevindavies8
test: create tools/eslint-format
e9735af 
Improve JS linting and add option to fix linting errors

Fixes: nodejs/node-addon-api#1076
PR-URL: nodejs/node-addon-api#1080
Reviewed-By: Michael Dawson <midawson@redhat.com
Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this pull request Aug 26, 2022
@Marlyfleitas
test: create tools/eslint-format
c764825 
Improve JS linting and add option to fix linting errors

Fixes: nodejs/node-addon-api#1076
PR-URL: nodejs/node-addon-api#1080
Reviewed-By: Michael Dawson <midawson@redhat.com
wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this pull request Sep 19, 2022
@wroy7860
test: create tools/eslint-format
ca751c0 
Improve JS linting and add option to fix linting errors

Fixes: nodejs/node-addon-api#1076
PR-URL: nodejs/node-addon-api#1080
Reviewed-By: Michael Dawson <midawson@redhat.com
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this pull request Aug 11, 2023
test: create tools/eslint-format
58b2329 
Improve JS linting and add option to fix linting errors

Fixes: nodejs/node-addon-api#1076
PR-URL: nodejs/node-addon-api#1080
Reviewed-By: Michael Dawson <midawson@redhat.com
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@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.

Follow up on adding JS linter

2 participants

@rubiagatra @mhdawson