gruntjs / grunt-contrib-jshint

Validate files with JSHint.
http://gruntjs.com/
MIT License
709 stars 193 forks source link

Support telling us what line and what file the error is commig from #240

Open paladox opened 8 years ago

paladox commented 8 years ago

Please support telling us which line and which file the error is coming from instead of just showing code and saying the error is in

this number off files. Since if you have a lot of files you wont know which file is doing it.

grunt-jscs and grunt-jsonlint support this.

paladox commented 8 years ago

This should be high priority since without telling you this information everything gets harder to fix the jshint errors.

vladikoff commented 8 years ago

@paladox could you please provide the current and the expected output. Thanks!

paladox commented 8 years ago

@vladikoff

Currently when it lints the file and finds some errors it does this

Actual outcome

19:13:40 Running "jshint:all" (jshint) task 19:13:56 19:13:56 19:13:56 219 | var getDefaultUri = ( function () { 19:13:56 ^ 'getDefaultUri' was used before it was defined. 19:13:56 19:13:56 87 | function ForeignTitle() { 19:13:56 ^ 'ForeignTitle' was used before it was defined. 19:13:56 19:13:56 >> 2 errors in 213 files 19:13:56 Warning: Task "jshint:all" failed. Use --force to continue. 19:13:56 19:13:56 Aborted due to warnings.

Expected outcome is so it looks a bit like this.

Th19:30:47 Running "jscs:all" (jscs) task

19:31:11 Illegal trailing whitespace at ./resources/src/mediawiki/mediawiki.Uri.js : 19:31:11 144 | 19:31:11 145 | var getDefaultUri; 19:31:11 146 |
19:31:11 ----------^ 19:31:11 147 | /* 19:31:11 148 | \ Construct a new URI object. Throws error if arguments are illegal/impossible, or 19:31:11 Illegal trailing whitespace at ./resources/src/mediawiki/mediawiki.Uri.js : 19:31:11 218 | } 19:31:11 219 | } 19:31:11 220 |
19:31:11 ----------^ 19:31:11 221 | getDefaultUri = ( function () { 19:31:11 222 | // Cache 19:31:11 >> 2 code style errors found! 19:31:11 Warning: Task "jscs:all" failed. Use --force to continue.

This is how grunt-jscs does it and the expected outcome for this grunt plugin. It shows you the line and file the error is comming form.

paladox commented 8 years ago

So it tells you the file and line the error is in. But it dosent in grunt-contrib-jshint please could this be changed so it can because grunt-jscs does and grunt-jsonlint does.

rahul-sekhar commented 8 years ago

I am also seeing this problem. Previously, this was working fine - I could see what file each error was coming from.

Now I don't see a filename above each error. Suppose I have 4 errors in 2 files out of a total of 15 files. It should say 4 errors in 2 files at the end, but now it says 4 errors in 15 files.

I haven't been able to tie this to a package version. Even on downgrading to 0.11.3 it continues to happen, so perhaps its a dependency version?

paladox commented 8 years ago

It has the line the error is comming from but doesn't tell your the file. Also we should add --> to tell you which piece of the code is actually causing the error please.

vladikoff commented 8 years ago

Previously, this was working fine

Yeah seems like a regression, probably a change in naming. Should be an easy pull request

paladox commented 8 years ago

I tested with 0.11.3 and it dosent work.

So this is supported just the code needs updating.

paladox commented 8 years ago

@vladikoff Could you open the pull since you know how and where to fix it please.

paladox commented 8 years ago

Yes it worked before but it seems to depend on which repo you test for example I am testing it on mediawiki/core which it wont show which file has the error but if I test it in mediawiki/extensions/CollapsibleVector it shows the error in which file.

stefcameron commented 8 years ago

@vladikoff This is definitely a regression. Without the path to the file, the output is pretty much useless. This was working at least in grunt-contrib-jshint version 0.11.3. No longer works in 0.12.0.

stefcameron commented 8 years ago

So here's the offending line: https://github.com/gruntjs/grunt-contrib-jshint/blob/master/tasks/lib/jshint.js#L95

If the ".bold" statement is removed, file paths start showing-up again using the default Grunt reporter.

This is with the latest version (0.12.0). Is see the same line in 0.11.3 code, but strangely it works when I run the plugin from a tree I installed about 3 months ago, but when I force a new tree to install version 0.11.3, the paths don't show-up. AFAIKT, both 0.11.3 trees are using the same code and same dependencies, but it's hard to tell. Obviously, there's still a difference somewhere. Perhaps there's a subtle difference in the many dependencies that grunt 0.4.5 references since those are all using version ranges, so I'm likely using a slightly different grunt in my new tree I just installed vs my old tree?

vladikoff commented 8 years ago

@stefcameron thanks for investigating, checking ...

vladikoff commented 8 years ago

@stefcameron I just reinstalled the modules, I see the file name here if I remove a semicolon somewhere in the file:

tasks/lib/jshint.js below

➜  grunt-contrib-jshint git:(master) ✗ npm test

> grunt-contrib-jshint@0.12.1 test /Users/vladikoff/dev/grunt-contrib-jshint
> grunt test

Running "jshint:allFiles" (jshint) task

   tasks/lib/jshint.js
     96 |        grunt.log.writeln((result.file ? '   ' + result.file : '').bold)
                                                                                 ^ Missing semicolon.

>> 1 error in 4 files
Warning: Task "jshint:allFiles" failed. Use --force to continue.

Aborted due to warnings.
npm ERR! Test failed.  See above for more details.
vladikoff commented 8 years ago

could be related to https://github.com/jshint/jshint/issues/2841

Different reporters?

paladox commented 8 years ago

Yes it seemed to work for me on some repos but on others it didn't.

Seems to be a bug then.

stefcameron commented 8 years ago

@vladikoff Hmmm... I'm not intimately familiar with all the inner components involved. I tried uninstalling grunt-contrib-jshint, cleaning npm cache, reinstall grunt-contrib-jshint, but I still see the issue of missing file paths.

The jshint issue you pointed to sounds very similar.

Perhaps for now remove ".bold" until the root cause is found? I'm not sure where this property comes from, especially on a string. I guess something somewhere is mixing some formatting properties on the string object, but I don't know how it's all supposed to work.

Splaktar commented 8 years ago

Using https://github.com/sindresorhus/jshint-stylish as the reporter seems to be a good workaround for now.

paladox commented 8 years ago

Actually https://github.com/sindresorhus/jshint-stylish looks nice we should use that since it is clearly explained which line and file and is in style.

But doesn't show the code that is causing it just the line and file.

stefcameron commented 8 years ago

@vladikoff Running jshint from the command straight out of "node_modules\grunt-contrib-jshint\node_modules.bin\jshint" uses jshint's default console reporter which logs a simple string to console.log() here: https://github.com/jshint/jshint/blob/master/src/reporters/default.js#L31 It works great, and the file name is output. So I'm not convinced this issue is related to https://github.com/jshint/jshint/issues/2841.

I think this issue is specifically related to the ".bold" property access I pointed-out earlier in this thread. I can't figure-out where the JavaScript String class is getting augmented with such a property (which I presume is intended to produce bolded console output), but all I can conclude is that for whatever reason, the mixin isn't working (at least for some of us) and accessing someString.bold results in undefined, and grunt.log.writeln(undefined) must yield an empty string, hence the missing file path...

My vote here is to simply remove ".bold"...

stefcameron commented 8 years ago

Interestingly, when running this plugin from within SublimeText and having its output to go Sublime's console, the file paths come out as function bold() { [native code] } strings (instead of the file paths). So .bold is defined then on the native string object, but it's a function to call instead of a property to read...?

...Yes! Now we're getting somewhere. Changing line https://github.com/gruntjs/grunt-contrib-jshint/blob/master/tasks/lib/jshint.js#L95 to grunt.log.writeln((result.file ? ' ' + result.file : '').bold()); results in this output to my Windows console (where the path was previously not output): <b> path/to/file.js</b> (same now in Sublime).

At least that's a string with the path so grunt.log.writeLn() now outputs it (instead of a Function object which outputs as an empty string, hence no path). But I suspect the output is supposed to be some ASCII codes telling the console to make the line bold instead of what looks like HTML formatting... I still have no clue where or what is supposed to be augmenting the native String object to have this mysterious .bold() method. Whatever is doing that, it must have changed its behavior, and grunt-contrib-jshint needs to be updated too.

shama commented 8 years ago

@stefcameron .bold comes from colors which modifies the String.prototype. I think the plugin you have in SublimeText is misleading you. The current syntax is correct.

But I think we should switch that out for chalk and avoid modifying the string prototype.

bgannonPL commented 8 years ago

Just updated and running into this issue. Without references to the file, jshint's usefulness drops considerably. It doesn't sound like there's a workaround at this point, is that correct?

rahul-sekhar commented 8 years ago

As mentioned above, using https://github.com/sindresorhus/jshint-stylish as the reporter is an easy immediate workaround to this.