Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Ignore a specific line #870

Closed
constfun opened this Issue Feb 21, 2013 · 28 comments

Comments

Projects
None yet

A feature to succinctly ignore a specific line would be useful. "I know what I'm doing right here linter, now shut up."

flake8 does it right. You can append "// NOQA" at the end of the line that you want to ignore and flake8 will not lint that specific line.

Example:

var s;
while(s = iterator.next()) { // NOQA
    ...
}

This is much cleaner and safer than the solutions poposed in the comments under #154. Ignoring a whole block of code completely defeats the purpose of using a linter. And having to surround the offending line in some sort of /jshint: .../ feels overkill for such a simple task.

glebm commented Apr 8, 2013

+1, this is especially important for hand-optimized code

I'd love to see this as well. It's not uncommon to inline minified library code at the top of a file, and currently this prevents it from passing JSHint. I've seen at least one case where the parser chokes entirely, part-way through the minified line, and won't continue on to the rest of the file.

Contributor

guyzmo commented Apr 12, 2013

When jshint "chokes" it usually means that it crashed, which is worth a bug report :-)

I looked at the log again, and actually I was mistaken; it did complete.

@guyzmo guyzmo referenced this issue in google/traceur-compiler Apr 15, 2013

Open

Use JSHint? #253

j201 commented Jul 2, 2013

+1
This would be great for when it's necessary to add minified code.

We develop an extension for Firefox and use jshint on all its files. However, Firefox uses some non-standard (or sometimes deprecated) javascript such as "for each" (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for_each...in) or "Array.forEach(function ([param1, param2]) {});".
Since we cannot avoid using this code, we would really appreciate an option to tell jshint to ignore certain lines of code.

+1

bfx81 commented Sep 4, 2013

+1

@ghost

ghost commented Sep 7, 2013

Code Bounty

danielmiladinov added a commit to danielmiladinov/jshint that referenced this issue Sep 8, 2013

danielmiladinov added a commit to danielmiladinov/jshint that referenced this issue Sep 8, 2013

Fixes #826, Fixes #870 - Add Directive `jshint ignore`.
This adds the ability to cause jshint to ignore either a block of lines
or just a single line.

You can can denote the start of an ignored block like this:

/*jshint ignore:start */

Lines inside an ignored block will be effectively invisible to the
linter. They aren't even presented to the lexer for tokenization.
This is ideal for blocking out sections of code that don't even remotely
resemble javascript, such as inline JSX

(http://facebook.github.io/react/docs/jsx-in-depth.html).

Similarly, you denote the end of an ignored block like this:

/*jshint ignore:end */

You can also ignore a single line with a trailing comment:

// jshint ignore:line

However, the use case presented for ignoring a single line seemed to be
more for silencing one or more warnings / errors in what was otherwise
valid javascript code (contrasted with JSX, which will never execute as
javascript without a transformation step to javascript). The linter will
still complain of errors if the lexer can't parse out the comment token.

danielmiladinov added a commit to danielmiladinov/jshint that referenced this issue Sep 8, 2013

Fixed #826, Fixed #870 - Add Directive `jshint ignore`.
This adds the ability to cause jshint to ignore either a block of lines
or just a single line.

You can can denote the start of an ignored block like this:

/*jshint ignore:start */

Lines inside an ignored block will be effectively invisible to the
linter. They aren't even presented to the lexer for tokenization.
This is ideal for blocking out sections of code that don't even remotely
resemble javascript, such as inline JSX

(http://facebook.github.io/react/docs/jsx-in-depth.html).

Similarly, you denote the end of an ignored block like this:

/*jshint ignore:end */

You can also ignore a single line with a trailing comment:

// jshint ignore:line

However, the use case presented for ignoring a single line seemed to be
more for silencing one or more warnings / errors in what was otherwise
valid javascript code (contrasted with JSX, which will never execute as
javascript without a transformation step to javascript). The linter will
still complain of errors if the lexer can't parse out the comment token.

Basically the same argument as @jsignanini - but for ES.next / ES6-Draft hinting.

The Use-Case here is using generators, natively in Google-Chrome Canary and Node.js unstable. The code gets Traceur-compiled for ES5 environments.

Some valid, uncompiled, syntax is still causing problems -- which is totally understandable while implementing a moving target spec -- but being able to ignore those false-positives for the time being would be very much appreciated.

Currently I'm "grep -v"-ing them from the output, which gets tedious quickly...

Member

caitp commented Sep 23, 2013

I don't mind taking a stab at this tomorrow -- What would be the most agreeable syntax for a comment to ignore a specific line?

... //NOHINT
/*NOLINT*/ ....

That sort of thing? I'm not sure if NOQA comes off very well.

Contributor

danielmiladinov commented Sep 23, 2013

@caitp, does my pull request not do what you want? Is it somehow not doing enough or perhaps doing the wrong thing?

Member

caitp commented Sep 23, 2013

@danielmiladinov I did not see your patch -- it seems a bit verbose though

Contributor

danielmiladinov commented Sep 23, 2013

Perhaps it's a bit verbose, but I don't it's too much so.

Sure, there's some pain in having to write all of this: // jshint ignore:line to ignore a line - and I think this is a good thing, because you're essentially telling the linter to not do its job when using this option. I hope that this discourages over-use.

If we make it too easy to disable linting or it's used too often, things could get to a point where you have to start asking yourself why you're using a linter to begin with.

That being said, if you're in a situation where you have to write code that otherwise doesn't lint, or probably won't ever lint, then I think the pain of using this as a trap door is still less than all the false positive errors / warnings you'd be getting otherwise.

Member

caitp commented Sep 23, 2013

Yeah these are good points, I just personally would prefer something a bit tinier -- but the jshint prefix is a good idea for sure. Anyway, I won't write a patch then, I'll just let folks review yours. Looks okay

@danielmiladinov I like your patch +1

+1

Being able to skip over an inlined library would be wonderful

valueof added a commit that referenced this issue Oct 17, 2013

Fixed #826, fixed #870: Add jshint ignore directive
Signed-off-by: Anton Kovalyov <anton@kovalyov.net>
Owner

valueof commented Oct 17, 2013

Fixed and merged in #1260.

@valueof valueof closed this Oct 17, 2013

vance commented Sep 30, 2014

How would you fix this without being completely unreadable? If you take out the if( node ) you get rule violation for wrap for with if... if you leave it in, you get nested too deep. if you break it apart, it is ugly and unreadable. I'm sure there's a prettier functional style to do this, but i've been codin' too longz today.

     for( var node in newList )
                  {
                    if( node ){

                       for( var oldNode in oldList )
                       {
                           if( oldNode )
                           {
                               if( newList[node].name === oldList[oldNode].name && oldList[oldNode].value === true)
                               {
                                   newList[node].value = true;
                               }
                           }

                       }
                    }
                }
Owner

rwaldron commented Sep 30, 2014

How would you fix this without being completely unreadable?

I'm tempted to troll you about the Allman style braces ;)

Try this:

/* jshint ignore: start */
for (var node in newList) {
  if (node) {
    for (var oldNode in oldList) {
      if (oldNode) {
        if (newList[node].name === oldList[oldNode].name && oldList[oldNode].value === true) {
          newList[node].value = true;
        }
      }
    }
  }
}
/* jshint ignore: end */

@rwaldron rwaldron reopened this Sep 30, 2014

@rwaldron rwaldron closed this Sep 30, 2014

vance commented Sep 30, 2014

ignore is no longer accepted, it causes a jshint error. Believe me, i tried that. Don't workey!

vance commented Sep 30, 2014

Yeah, what I did was
/* jshint maxdepth: 9 /
/
jshint maxcomplexity: 9*/

but it would be way easier if ignore actually worked. When you have automation and maintenance developers fixing your stuff, sticking to Allman style breaks is not only better, but cheaper.

vance commented Sep 30, 2014

I posted that as a brainteaser in our daily-stand up meeting email.

jugglinmike added a commit to jugglinmike/jshint that referenced this issue Oct 21, 2014

Fixed #826, fixed #870: Add jshint ignore directive
Signed-off-by: Anton Kovalyov <anton@kovalyov.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment