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.

Sign up for GitHub

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

Jump to bottom

tools: add recommended linting rules #5188

Closed
wants to merge 1 commit into from
Closed

tools: add recommended linting rules #5188

wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Feb 10, 2016

This change adds ESLint rules that meet two criteria:

  • recommended by ESLint
  • require no code changes in the current code base

These rules are:

  • no-func-assign: Disallow overwriting a function that was written
    as a function declaration.
  • no-negated-in-lhs: Disallow negated left operand of in operator.
    It prevents if(!a in b) when if(!(a in b)) is intended.
  • no-obj-calls: Disallow global object function calls. It prevents
    errors like JSON() and Math().
  • use-isnan: Prevents errors like if (foo == NaN)
  • no-octal: Disallows confusing constructs like var num = 071;
  • no-delete-var: Delete works on properties, not variables. Disallows
    delete foo.

This may make configuration much more concise if we turn on recommended rules and then disable ones that we aren't using.

And it may prevent some errors from making it into the PR stage and requiring a reviewer to correct.

@Trott Trott added tools Issues and PRs related to the tools directory. lts-watch-v4.x labels Feb 10, 2016
@Trott
Copy link
Member Author

Trott commented Feb 10, 2016

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

@thefourtheye
Copy link
Contributor

LGTM but regarding isNaN I remember @bnoordhuis proposing a patch to use != as it has performance impact.

@Trott
Copy link
Member Author

Trott commented Feb 11, 2016

@thefourtheye I may be misunderstanding, but I don't think that would work:

var foo = NaN;
console.log(foo != NaN); // oops, logs "true"

Anything (including NaN itself) compared to NaN returns false, so a comparison with a hardcoded NaN value is usually (always?) a coding error.

@thefourtheye
Copy link
Contributor

foo !== foo will work and even spec says it.

@silverwind
Copy link
Contributor

foo !== NaN could very well be optimized to true 😉

@Trott
Copy link
Member Author

Trott commented Feb 11, 2016

@thefourtheye wrote:

foo !== foo will work and even spec says it.

Ah! I see! Well, fortunately, the linting rule won't flag that. So everybody wins!

@targos
Copy link
Member

targos commented Feb 11, 2016

@thefourtheye foo !== foo is not affected by this rule.

@targos
Copy link
Member

targos commented Feb 11, 2016

LGTM. Can you also add a line about no-func-assign in the commit message?

@thefourtheye
Copy link
Contributor

@thefourtheye foo !== foo is not affected by this rule.

@targos I understand that. I was actually referring to the performance drop mentioned in #1925

@Trott
tools: add recommended linting rules
dd7fb4d 
This change adds ESLint rules that meet two criteria:

* recommended by ESLint
* require no code changes

These rules are:

* `no-func-assign`: Disallow overwriting a function that was written
as a function declaration.
* `no-negated-in-lhs`: Disallow negated left operand of `in` operator.
It prevents `if(!a in b)` when `if(!(a in b))` is intended.
* `no-obj-calls`: Disallow global object function calls. It prevents
errors like `JSON()` and `Math()`.
to exercise the code in tests or whatever, it can sneak in.
* `use-isnan`: Prevents errors like `if (foo == NaN)`
* `no-octal`: Disallows confusing constructs like `var num = 071;`
* `no-delete-var`: Delete works on properties, not variables. Disallows
`delete foo`.
@Trott
Copy link
Member Author

Trott commented Feb 11, 2016

@targos Whoops, missed that one! I've updated the commit message.

Trott added a commit that referenced this pull request Feb 12, 2016
@Trott
tools: add recommended linting rules
ffbc05a 
This change adds ESLint rules that meet two criteria:

* recommended by ESLint
* require no code changes

These rules are:

* `no-func-assign`: Disallow overwriting a function that was written
as a function declaration.
* `no-negated-in-lhs`: Disallow negated left operand of `in` operator.
It prevents `if(!a in b)` when `if(!(a in b))` is intended.
* `no-obj-calls`: Disallow global object function calls. It prevents
errors like `JSON()` and `Math()`.
to exercise the code in tests or whatever, it can sneak in.
* `use-isnan`: Prevents errors like `if (foo == NaN)`
* `no-octal`: Disallows confusing constructs like `var num = 071;`
* `no-delete-var`: Delete works on properties, not variables. Disallows
`delete foo`.

PR-URL: #5188
Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@Trott
Copy link
Member Author

Trott commented Feb 12, 2016

Landed in ffbc05a

@Trott Trott closed this Feb 12, 2016
rvagg pushed a commit that referenced this pull request Feb 15, 2016
@Trott @rvagg
tools: add recommended linting rules
b611caa 
This change adds ESLint rules that meet two criteria:

* recommended by ESLint
* require no code changes

These rules are:

* `no-func-assign`: Disallow overwriting a function that was written
as a function declaration.
* `no-negated-in-lhs`: Disallow negated left operand of `in` operator.
It prevents `if(!a in b)` when `if(!(a in b))` is intended.
* `no-obj-calls`: Disallow global object function calls. It prevents
errors like `JSON()` and `Math()`.
to exercise the code in tests or whatever, it can sneak in.
* `use-isnan`: Prevents errors like `if (foo == NaN)`
* `no-octal`: Disallows confusing constructs like `var num = 071;`
* `no-delete-var`: Delete works on properties, not variables. Disallows
`delete foo`.

PR-URL: #5188
Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 18, 2016
@Trott
tools: add recommended linting rules
d3185c8 
This change adds ESLint rules that meet two criteria:

* recommended by ESLint
* require no code changes

These rules are:

* `no-func-assign`: Disallow overwriting a function that was written
as a function declaration.
* `no-negated-in-lhs`: Disallow negated left operand of `in` operator.
It prevents `if(!a in b)` when `if(!(a in b))` is intended.
* `no-obj-calls`: Disallow global object function calls. It prevents
errors like `JSON()` and `Math()`.
to exercise the code in tests or whatever, it can sneak in.
* `use-isnan`: Prevents errors like `if (foo == NaN)`
* `no-octal`: Disallows confusing constructs like `var num = 071;`
* `no-delete-var`: Delete works on properties, not variables. Disallows
`delete foo`.

PR-URL: #5188
Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 18, 2016
@Trott
tools: add recommended linting rules
993d9b7 
This change adds ESLint rules that meet two criteria:

* recommended by ESLint
* require no code changes

These rules are:

* `no-func-assign`: Disallow overwriting a function that was written
as a function declaration.
* `no-negated-in-lhs`: Disallow negated left operand of `in` operator.
It prevents `if(!a in b)` when `if(!(a in b))` is intended.
* `no-obj-calls`: Disallow global object function calls. It prevents
errors like `JSON()` and `Math()`.
to exercise the code in tests or whatever, it can sneak in.
* `use-isnan`: Prevents errors like `if (foo == NaN)`
* `no-octal`: Disallows confusing constructs like `var num = 071;`
* `no-delete-var`: Delete works on properties, not variables. Disallows
`delete foo`.

PR-URL: #5188
Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 18, 2016
Merged
stefanmb pushed a commit to stefanmb/node that referenced this pull request Feb 23, 2016
@Trott @stefanmb
tools: add recommended linting rules
553439c 
This change adds ESLint rules that meet two criteria:

* recommended by ESLint
* require no code changes

These rules are:

* `no-func-assign`: Disallow overwriting a function that was written
as a function declaration.
* `no-negated-in-lhs`: Disallow negated left operand of `in` operator.
It prevents `if(!a in b)` when `if(!(a in b))` is intended.
* `no-obj-calls`: Disallow global object function calls. It prevents
errors like `JSON()` and `Math()`.
to exercise the code in tests or whatever, it can sneak in.
* `use-isnan`: Prevents errors like `if (foo == NaN)`
* `no-octal`: Disallows confusing constructs like `var num = 071;`
* `no-delete-var`: Delete works on properties, not variables. Disallows
`delete foo`.

PR-URL: nodejs#5188
Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
@Trott
tools: add recommended linting rules
179b811 
This change adds ESLint rules that meet two criteria:

* recommended by ESLint
* require no code changes

These rules are:

* `no-func-assign`: Disallow overwriting a function that was written
as a function declaration.
* `no-negated-in-lhs`: Disallow negated left operand of `in` operator.
It prevents `if(!a in b)` when `if(!(a in b))` is intended.
* `no-obj-calls`: Disallow global object function calls. It prevents
errors like `JSON()` and `Math()`.
to exercise the code in tests or whatever, it can sneak in.
* `use-isnan`: Prevents errors like `if (foo == NaN)`
* `no-octal`: Disallows confusing constructs like `var num = 071;`
* `no-delete-var`: Delete works on properties, not variables. Disallows
`delete foo`.

PR-URL: #5188
Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@Trott Trott deleted the eslint-moar branch January 13, 2022 22:42
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
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.
5 participants
@Trott @thefourtheye @silverwind @targos @MylesBorins