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

create tools/eslint-format #1080

Closed
wants to merge 2 commits into from
Closed

create tools/eslint-format #1080

wants to merge 2 commits into from

Conversation

rubiagatra
Copy link
Contributor

@rubiagatra rubiagatra commented Sep 29, 2021

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
Member

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

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

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

@rubiagatra
Copy link
Contributor Author

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
Contributor Author

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

@rubiagatra
Copy link
Contributor Author

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

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

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 pushed a commit that referenced this pull request Nov 15, 2021
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
Member

Landed in 706b199

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

thanks! @mhdawson

kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this pull request Aug 24, 2022
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
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
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
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Follow up on adding JS linter
2 participants