Switch from jshint to eslint

[paulmelnikow]

This is using eslint:recommended, which provides a bunch of mostly style-agnostic rules (look for the ✔️'s). Plus a couple extra whitespace rules, and strict mode. no-console is turned off for tests and examples to avoid needing a bunch of overrides. It's a moderately large diff, primarily on account of a mass of unused variables in test_intercept.js.

There's not much changing of substance though.

I've got one failing unit test. It's been passing erroneously because scope.done() on line 161 causes a ReferenceError. (scope is not defined.) It's not immediately clear how to fix it.

I ran the .jshintrc through polyjuice. Some of the rules it generated would have produced large diffs, so I added them to eslintrc but left them commented out. We can turn them back on in subsequent passes. We may also want to look for a shared config.

For #868

---

Summary

Switch from jshint to eslint

• Remove pre-commit hooks
• Fix a broken unit test (bug caught by lint)
• Minor lint and style cleanup

Closes #868

[paulmelnikow]

Hi there, any update on this? To avoid conflicts, would be great to get it merged before much else goes in.

[paulmelnikow]

Still have a failing unit test here, in test_back.js. It's been passing erroneously because scope.done() on line 161 211 causes a ReferenceError. (scope is not defined.) It's not immediately clear how to fix it.

Do you understand what the assertion is supposed to be doing?

[paulmelnikow]

^^ Since scope is not defined, this line caused a ReferenceError, which erroneously caused the test to pass.

[gr2m]

I’ve looked into the test, and they try/catch doesn’t make any sense to me, so I’ll just remove it ¯\_(ツ)_/¯ 58c120c

[gr2m]

how about we put npm run lint into a "pretest" script or "posttest" script in package.json, so it automatically gets run with tests? That’s what I usually do

[paulmelnikow]

Makes sense to me! I didn't realize it wasn't in there. It should definitely get run with npm run test.

[paulmelnikow]

Looks like it is getting run with npm test!

[gr2m]

Ah I see it now. But we should probably add npm run unit to the list?

And move npm run coverage to after_success?

[paulmelnikow]

npm run unit: we sure should! Hahaha.

I'm inclined to run npm run coverage in after_script rather than after_success. Coverage data is still useful, even if a couple unit tests fail or a lint rule fails.

[gr2m]

great work 👍

[gr2m]

Hey @paulmelnikow thanks for helping out with answering issues and reviewing Pull Request! I’d be happy to make you part of the maintainers team if you like? That would allow you to actually review & merge pull requests and create your own pull requests from branches instead of forks

[paulmelnikow]

@gr2m I'd be down! I depend on nock for shields, and feel it's an important dependency for so much of the community. I've dug into it before and am glad to keep doing that. While I'm not sure how consistently I'll be able to help, I'm glad to do it when I can!

[gr2m]

No worries, there are no expectations, it’s all volunteer work and life happens. Just keep us posted :) And us know if you have any ideas on how to move the project forward towards a more open, contributor friendly project :)

I’ve invited you, you should see the notification on https://github.com/node-nock

[gr2m]

looks like it’s currently failing due to a linting error :)

[paulmelnikow]

Huh. Most strange. That most recent test build ran on the previous commit, not my last commit. Lint is clean. 🤷‍♂️

[coveralls]

Coverage decreased (-0.02%) to 92.686% when pulling d16dc40 on paulmelnikow:eslint into b1e8787 on node-nock:master.

[paulmelnikow]

  "pre-commit": [
    "jshint",
    "coverage"
  ],

This seems way excessively frequently to run these and they slow down my workflow a bunch. I end up using --no-verify all the time, but that also means I can't make commits in the graphical Git tool I also use.

Frankly I'm inclined to move lint to pre-push and let coverage (i.e. unit tests) be handled in CI. Thoughts?

[gr2m]

the current lint errors are legit, I’m looking into it. Let’s keep it in CI, but there are ways to speed things up that we can look into later :)

[coveralls]

Coverage decreased (-0.02%) to 92.686% when pulling 5ab4443 on paulmelnikow:eslint into b1e8787 on node-nock:master.

[paulmelnikow]

Ah, I'm not suggesting removing anything from CI, rather removing unit tests from the pre-commit hook, and moving lint from pre-commit to pre-push.

[gr2m]

ah got it, only seen the pre-commit hook, it doesn’t work for me anyway. I’d remove all the commit hooks, I’ve seen people had so many struggle with them, it’s not worth the pain

[coveralls]

Coverage decreased (-0.02%) to 92.686% when pulling 8cd6d9d on paulmelnikow:eslint into b1e8787 on node-nock:master.

[coveralls]

Coverage decreased (-0.02%) to 92.686% when pulling d42fb3b on paulmelnikow:eslint into b1e8787 on node-nock:master.

[gr2m]

good to go from my side :)

Maybe squash the commits when merging

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue and add a reference to this one if it’s related. Thank you!