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

repl, url, tools: use no-use-before-define ESLint rule #14032

Closed
wants to merge 2 commits into from
Closed

repl, url, tools: use no-use-before-define ESLint rule #14032

wants to merge 2 commits into from

Conversation

vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt commented Jul 1, 2017

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

repl, url, tools

Prehistory

In #14021, I've committed something like this by an oversight:

const assert = require('assert');
if (!common.hasCrypto)
  common.skip('missing crypto');

const common = require('../common');

assert and common lines was erroneously swapped. All tests have failed except linter.

ESLint has a rule for these cases: http://eslint.org/docs/rules/no-use-before-define

It has 3 options for classes, functions, and variables. I've left the class function on (defaults) as its violation is noted to be potentially dangerous. I've set the function option off as this is considered safe due to hoisting. And I've set the variable option off as we have many cases with cross-references from callbacks which are rather difficult to refactor. But with this option off, the rule will still check cases inside the same scope, like my above-mentioned error.

The first commit fixes 2 fragments in libs, the second one applies the rule.

@nodejs-github-bot nodejs-github-bot added dont-land-on-v4.x repl Issues and PRs related to the REPL subsystem. tools Issues and PRs related to the tools directory. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Jul 1, 2017
@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Jul 1, 2017

CI: https://ci.nodejs.org/job/node-test-pull-request/8920/

All failed and unstable results seem unrelated.

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Jul 4, 2017

Pre-land CI: https://ci.nodejs.org/job/node-test-pull-request/8979/

One arm fail seems unrelated.

@vsemozhetbyt
Copy link
Contributor Author

Landed in 5100cc6

@vsemozhetbyt vsemozhetbyt deleted the eslint-no-use-before-define branch July 4, 2017 17:28
vsemozhetbyt added a commit that referenced this pull request Jul 4, 2017
Also fix repl and url libs for the rule.

PR-URL: #14032
Refs: http://eslint.org/docs/rules/no-use-before-define
Reviewed-By: Daijiro Wachi <[email protected]>
addaleax pushed a commit that referenced this pull request Jul 11, 2017
Also fix repl and url libs for the rule.

PR-URL: #14032
Refs: http://eslint.org/docs/rules/no-use-before-define
Reviewed-By: Daijiro Wachi <[email protected]>
@addaleax addaleax mentioned this pull request Jul 11, 2017
addaleax pushed a commit that referenced this pull request Jul 18, 2017
Also fix repl and url libs for the rule.

PR-URL: #14032
Refs: http://eslint.org/docs/rules/no-use-before-define
Reviewed-By: Daijiro Wachi <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
Also fix repl and url libs for the rule.

PR-URL: #14032
Refs: http://eslint.org/docs/rules/no-use-before-define
Reviewed-By: Daijiro Wachi <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 15, 2017
Also fix repl and url libs for the rule.

PR-URL: #14032
Refs: http://eslint.org/docs/rules/no-use-before-define
Reviewed-By: Daijiro Wachi <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 15, 2017
Also fix repl and url libs for the rule.

PR-URL: #14032
Refs: http://eslint.org/docs/rules/no-use-before-define
Reviewed-By: Daijiro Wachi <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 15, 2017
Also fix repl and url libs for the rule.

PR-URL: #14032
Refs: http://eslint.org/docs/rules/no-use-before-define
Reviewed-By: Daijiro Wachi <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
Also fix repl and url libs for the rule.

PR-URL: #14032
Refs: http://eslint.org/docs/rules/no-use-before-define
Reviewed-By: Daijiro Wachi <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Aug 16, 2017
MylesBorins pushed a commit that referenced this pull request Sep 5, 2017
Also fix repl and url libs for the rule.

PR-URL: #14032
Refs: http://eslint.org/docs/rules/no-use-before-define
Reviewed-By: Daijiro Wachi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
repl Issues and PRs related to the REPL subsystem. tools Issues and PRs related to the tools directory. whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants