FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

tools: enable no-unused-vars linter rule and fix issues by silverwind · Pull Request #2828 · nodejs/node · GitHub

/ node Public

tools: enable no-unused-vars linter rule and fix issues - #2828

Closed
silverwind wants to merge 2 commits into
nodejs:masterfrom
silverwind:no-unused-vars
ConversationCommits (2)ChecksFiles changed
Closed

tools: enable no-unused-vars linter rule and fix issues#2828
silverwind wants to merge 2 commits into
nodejs:masterfrom
silverwind:no-unused-vars

Conversation

silverwind commented Sep 12, 2015

Copy link
Copy Markdown
Contributor

This enables the no-unused-vars rule. Issues that came up were mostly the variables on top of tests and there were a few cases where there was leftover dead code.

I intentionally didn't enable the feature for unused function arguments yet. I think the best to go about these is to comment out the arguments in lib while removing them in test, does that sound good?

Note that this is somewhat WIP, as there were a few cases where the intention seemed to be to use some of these variables, but I'm submitting this now as-is for some feedback.

silverwind added the tools Issues and PRs related to the tools directory. label Sep 12, 2015
Fishrock123 reviewed Sep 12, 2015
View reviewed changes
Comment thread test/internet/test-http-dns-fail.js Outdated

Fishrock123 Sep 12, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Requiring common actually just does things.

I suppose we don't need to assign it to a variable though.

silverwind Sep 12, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Are these 'things' relevant to tests that don't use the variable?

Fishrock123 Sep 12, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yes. See https://github.com/nodejs/node/blob/master/test/common.js#L304-L313 among other things. Actually, all tests should require common. (Perhaps it should just be pre-loaded, not sure.)

silverwind Sep 12, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

If it's just this leaked global check, I see two options:

  • We remove that leaked global check. These can be warned through the linter unless we do something like global.x, but that ought to be intentional.
  • We preload common.js

Trott Sep 12, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

There's this too but I think that's it:

node/test/common.js

Lines 142 to 151 in 68dc69a

if (process.env.NODE_COMMON_PIPE) {
exports.PIPE = process.env.NODE_COMMON_PIPE;
// Remove manually, the test runner won't do it
// for us like it does for files in test/tmp.
try {
fs.unlinkSync(exports.PIPE);
} catch (e) {
// Ignore.
}
}

rvagg Sep 12, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

what's the big deal with requiring common? are you trying to shave off ms?

silverwind Sep 12, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Requiring common without actually using it violates the rule I'm trying to add 😉

silverwind Sep 12, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Just saw that the rule allows exceptions per varsIgnorePattern, so that could be another option.

Trott Sep 12, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Could also change var common = require('../common.js') to just require('../common.js');...

silverwind Sep 12, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

That looks a bit strange imho and would confuse someone who doesn't know about this 'dependency'.

silverwind commented Sep 12, 2015

Copy link
Copy Markdown
Contributor Author

Looks like option I need was added just in 1.3 (eslint/eslint@91fc1c5), making this PR depend on #2286.

thefourtheye mentioned this pull request Sep 12, 2015
Closed
silverwind added the stalled Issues and PRs that are stalled. label Nov 16, 2015

silverwind commented Jan 6, 2016

Copy link
Copy Markdown
Contributor Author

This is obsolete now with #4536.

silverwind closed this Jan 6, 2016
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

stalled Issues and PRs that are stalled. tools Issues and PRs related to the tools directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL