Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Add a poorrelation option to ignore "Use '{a}' to compare with '{b}'." warnings. #511

Closed
wants to merge 3 commits into from

5 participants

Kristof Neirynck Anton Kovalyov Matt Perpick Ben Hockey Josh Perez
Kristof Neirynck

Added an ignore rule for isPoorRelation warnings and named it poorrelation.

Anton Kovalyov
Owner

Thanks for the patch. I have a couple of comments:

  • Could you give us an example code where this option is needed and eqnull can't satisfy the requirements?
  • Consider adding the poorrelation check into isPoorRelation instead of duplicating it throughout the code:
function isPoorRelation(node) {
    return !option.poorrelation && node &&
        ((node.type === '(number)' && +node.value === 0) ||
        (node.type === '(string)' && node.value === '') ||
        (node.type === 'null' && !option.eqnull) ||
        node.type === 'true' ||
        node.type === 'false' ||
        node.type === 'undefined');
}
Kristof Neirynck

Hi Anton,

I've inherited some legacy code that contained quite a few bugs.
There were a million warnings about comparisons with empty strings, false and undefined.
Adding the poorrelation option helped me filter those out.

I've moved the poorrelation check into isPoorRelation as you requested.

Anton Kovalyov
Owner

Hey Kristof,

Thanks for the update. My problem with this proposed option is that JSHint cannot recommend it in any way—but it is useful in your case, when you have to lint legacy code. So I'm thinking about hiding this and similar options under some kind of a flag. For example, what if JSHint allowed the poorrelation option only if you specifically indicate that you're linting a legacy code?

JSHINT({
  code: "...",
  options: { poorrelation: true },
  globals: {},
  legacy: true
});

And for CLI version:

jshint --legacy mylegacyfile.js

CC @goatslacker @WolfgangKluge

Anton Kovalyov
Owner

On the other hand, all these options for legacy code kind of defeat the purpose of the linter. We need a sweet spot between being practical and being helpful.

Matt Perpick

I'm also adding jshint to an existing codebase with plenty of lint. Rather than ignore a few errors in particular particular, I'd rather see see the node utility support warnings - meaning print the lint error, but return an exit status of zero if only warnings are encountered.

I feel like it could be implemented in a relatively backwards compatible way. Something like ...

{  
   // Use the standard jslint config ... 
   white: true,
   eqnull: true,

   // But add a warnings section ...
   warningRules : ['white', 'eqnull', ...]
}

// Run the standard jslint (pseudo-code obviously)
var results = lint(files, config);
report(results);
// But don't fail the process if the errors on rules we'd like to temporarily ignore.
var returnCode = 0;
results.forEach(function (result) {
    if (result.rule not in warningRules) {
          returnCode = 1;
    }
});
process.exit(returnCode);

That would make it really easy to roll out jshint in big code bases, because folks can get feedback on their code and can include jshint in continuous integration immediately with a lax set of rules and gradually tighten up code and start removing warnings.

Matt Perpick

But still, it's good to have the poor relation config too :)

Anton Kovalyov
Owner

I am very sorry but I've decided against this patch. Your code looks pretty good but I am afraid this option can be abused to the point where it'll defeat the whole point of linting your code. FWIW, JSHint Next will allow you to hide/ignore any warning so your case will be covered.

Anton Kovalyov valueof closed this
Kristof Neirynck

I'm afraid this makes jshint stricter than jslint.
Which kind of feels weird.
I switched to jshint because it was more lenient.
It allows "for (var i=0" which jslint doesn't.
I'll look into jshint next code and try to backport the option from there.

For the time being I'll use my fork.

Anton Kovalyov
Owner

Hm, that's interesting. Does JSLint allow poor relations by default or is there an option?

Kristof Neirynck

Jslint doesn't have a "poorrelation" check.

Try running this code trough both jslint and jshint.

/*jshint eqnull:true */
/*jslint eqeq:true */
function dummy() {
    'use strict';
    var a = 1;
    if (a == 0 ||
            a == '' ||
            a == null ||
            a == true ||
            a == false ||
            a == undefined) {
        a = 2;
    }
}

Jshint is the only one to give me errors:

Line 6:     if (a == 0 ||
Use '===' to compare with '0'.
Line 7: a == '' ||
Use '===' to compare with ''.
Line 9: a == true ||
Use '===' to compare with 'true'.
Line 10: a == false ||
Use '===' to compare with 'false'.
Line 11: a == undefined) {
Use '===' to compare with 'undefined'.
Ben Hockey

did you try the eqeqeq option with jshint?

Ben Hockey

...which would be eqeqeq:false

Kristof Neirynck

eqeqeq:false doesn't help for poorrelations.

This code still reports the same errors.
You can check it by copy pasting it in http://www.jshint.com/

/*jshint eqnull:true, eqeqeq:false */
/*jslint eqeq:true */
function dummy() {
    'use strict';
    var a = 1;
    if (a == 0 ||
            a == '' ||
            a == null ||
            a == true ||
            a == false ||
            a == undefined) {
        a = 2;
    }
}
Josh Perez

I feel like this should warn but it doesn't.

/*jshint expr: true, eqeqeq: false, undef: false */
a == 1;
Anton Kovalyov
Owner

It shouldn't. eqeqeq:true will require strict comparison for all. Without it we check only potentially dangerous cases such as == null, == 0 and so on.

Josh Perez

Isn't the point of warning on false and true because of coercion?

/*jshint expr: true, eqeqeq: false, undef: false */
a == true; // fails
a == 0; // fails
a == false; // fails
a == 1; // passes
Anton Kovalyov
Owner

Yeah. I think we're misunderstanding each other.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 6, 2012
  1. Kristof Neirynck
  2. Kristof Neirynck
Commits on Apr 7, 2012
  1. Kristof Neirynck

    adding the poorrelation check into isPoorRelation instead of duplicat…

    Crydust authored
    …ing it throughout the code
This page is out of date. Refresh to see the latest.
Showing with 3 additions and 2 deletions.
  1. +3 −2 jshint.js
5 jshint.js
View
@@ -214,7 +214,7 @@
moveTo, mootools, multistr, name, navigator, new, newcap, noarg, node, noempty, nomen,
nonew, nonstandard, nud, onbeforeunload, onblur, onerror, onevar, onecase, onfocus,
onload, onresize, onunload, open, openDatabase, openURL, opener, opera, options, outer, param,
- parent, parseFloat, parseInt, passfail, plusplus, predef, print, process, prompt,
+ parent, parseFloat, parseInt, passfail, plusplus, poorrelation, predef, print, process, prompt,
proto, prototype, prototypejs, provides, push, quit, range, raw, reach, reason, regexp,
readFile, readUrl, regexdash, removeEventListener, replace, report, require,
reserved, resizeBy, resizeTo, resolvePath, resumeUpdates, respond, rhino, right,
@@ -303,6 +303,7 @@ var JSHINT = (function () {
onecase : true, // if one case switch statements should be allowed
passfail : true, // if the scan should stop on first error
plusplus : true, // if increment/decrement should not be allowed
+ poorrelation: true, // if poor relations should be allowed
proto : true, // if the `__proto__` property should be allowed
prototypejs : true, // if Prototype and Scriptaculous globals should be
// predefined
@@ -2231,7 +2232,7 @@ loop: for (;;) {
function isPoorRelation(node) {
- return node &&
+ return !option.poorrelation && node &&
((node.type === '(number)' && +node.value === 0) ||
(node.type === '(string)' && node.value === '') ||
(node.type === 'null' && !option.eqnull) ||
Something went wrong with that request. Please try again.