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

Remove unused variables / add linting #748

Merged
merged 1 commit into from Mar 5, 2022
Merged

Conversation

jsumners
Copy link
Contributor

@jsumners jsumners commented May 3, 2021

In my last few PRs I have noticed that there are many unused variables across the code. This can lead to issues as we saw in #745. This PR adds eslint to catch these sorts of errors. The eslint configuration is based on the standard "eslint:recommended" rule set. I have disabled:

  • no-prototype-builtins
  • no-empty
  • no-regex-spaces
  • no-useless-escape

I have disabled these so that this PR has minimal impact on the code. I recommend someone who knows both the code and the regular expressions to re-enable the no-regex-spaces and no-useless-escape rules and fixing the code accordingly.

@@ -692,6 +691,7 @@ Much more documentation available at: https://www.node-tap.org/
appended to the filenames.`,
}),

/* eslint-disable-next-line */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what to do here. There's a lot of code in here and it's hard to see it all. So I just disable the warning. However, if it is accurate, then I think this key will take precedence over the first definition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be clear: this comment directive is disabling a "duplicate key" warning.

@jsumners
Copy link
Contributor Author

ping: @isaacs @coreyfarrell

PR-URL: tapjs#748
Credit: @jsumners
Close: tapjs#748
Reviewed-by: @isaacs

EDIT(@isaacs): move lint to postsnap, and do --fix by default.  We
always have to run snapshots before pushing a new version, and expect to
get code changes as a result.  This is much more convenient than having
tests just not run becuase of some scaffolding or something.
@isaacs isaacs closed this in 3f1ae47 Mar 5, 2022
@isaacs isaacs merged commit 3f1ae47 into tapjs:main Mar 5, 2022
@jsumners jsumners deleted the lint-code branch March 5, 2022 20:45
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.

None yet

2 participants