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

lib: remove unused modules #4396

Closed
wants to merge 1 commit into from
Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented Dec 23, 2015

Some files in lib were using require to load modules that were
subsequently not used in the file. This removes those require
statements.

Some files in `lib` were using `require` to load modules that were
subsequently not used in the file. This removes those `require`
statements.
@Trott Trott added http Issues or PRs related to the http subsystem. os Issues and PRs related to the os subsystem. tty Issues and PRs related to the tty subsystem. lib / src Issues and PRs related to general changes in the lib or src directory. labels Dec 23, 2015
@jbergstroem
Copy link
Member

@Trott
Copy link
Member Author

Trott commented Dec 23, 2015

ppcle-ubuntu1404 failure looks like Jenkins/build problem.

@Trott
Copy link
Member Author

Trott commented Dec 23, 2015

CI looks good other than aforementioned ppcle-ubuntu1404 issue that seems unrelated.

@jbergstroem
Copy link
Member

It's 99.999% my bad; currently rewriting init scripts in upstart so they respawn if they go down. Not sure why the iojs user would have permission issues in ~/ though.

Edit: DIAGFILE=${HOME}/jenkins_diagnostics.txt -- guessing HOME is unset.
Edit2: fixed.

@bnoordhuis
Copy link
Member

LGTM

@cjihrig
Copy link
Contributor

cjihrig commented Dec 23, 2015

LGTM. Any reason we aren't linting for unused variables?

@targos
Copy link
Member

targos commented Dec 23, 2015

@cjihrig we would need the varsIgnorePattern option to ignore common in tests. Unfortunately it doesn't exist yet in our current version of ESLint.

@Trott
Copy link
Member Author

Trott commented Dec 23, 2015

@cjihrig Linting for unused variables would require us fixing 700+ instances of the issue in the code. The ones I've submitted are uncontroversial ones, but some might result in some debate and discussion, so I'd like to do all the straightforward ones like this one first so the discussion is around the 70 or 80 (guessing) potentially-controversial ones and doesn't hold up the hundreds of ones that should sail through.

Here's an example of one that might result in a prolonged discussion: There are three unused vars in src/node.js including id at

NativeModule.isInternal = function(id) {
.
On the one hand, id isn't used in that function. On the other hand, removing it from the function signature may make it less clear that it is an analogue to the function with the same signature at
NativeModule.isInternal = function(id) {
. I personally think we should remove the unused variable and name the function if we want to make the analogous nature clear. But that's not quite as straightforward as "the variable in this var statement is never used, so I'm removing it".

@cjihrig
Copy link
Contributor

cjihrig commented Dec 23, 2015

The no-unused-vars rule also has an option to not check function arguments. That would help with at least some of the cases. But, yea, not being able to upgrade ESLint is a pain.

@Trott
Copy link
Member Author

Trott commented Dec 23, 2015

For common, we can just change var common = require(...) to just require(...). In other words, require it without assigning it to a variable.

@mscdex
Copy link
Contributor

mscdex commented Dec 23, 2015

Maybe I missed it but, what is the reason why we can't upgrade our copy of eslint?

@Trott
Copy link
Member Author

Trott commented Dec 23, 2015

@mscdex See #2286.

@silverwind
Copy link
Contributor

@mscdex Basically the way we did indentation is not compatible with the latest versions and to fix that we'd have to introduce around 174 lines of "churn" in lib which some people opposed for git blame reasons.

Trott added a commit to Trott/io.js that referenced this pull request Dec 25, 2015
Some files in `lib` were using `require` to load modules that were
subsequently not used in the file. This removes those `require`
statements.

PR-URL: nodejs#4396
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@Trott
Copy link
Member Author

Trott commented Dec 25, 2015

Landed in fdeb862

@Trott Trott closed this Dec 25, 2015
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Jan 6, 2016
Some files in `lib` were using `require` to load modules that were
subsequently not used in the file. This removes those `require`
statements.

PR-URL: nodejs#4396
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorins
Copy link
Contributor

@Trott it looks like these changes are breaking v4.x-staging. Would you be able to backport the changes and open a PR against v4.x-staging?

@Trott
Copy link
Member Author

Trott commented Jan 14, 2016

@thealphanerd #4683

scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
Some files in `lib` were using `require` to load modules that were
subsequently not used in the file. This removes those `require`
statements.

PR-URL: nodejs#4396
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@Trott Trott deleted the rm-unused-node-vars branch January 13, 2022 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem. lib / src Issues and PRs related to general changes in the lib or src directory. os Issues and PRs related to the os subsystem. tty Issues and PRs related to the tty subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants