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

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

Open
paladox opened this issue Jan 17, 2016 · 24 comments
Open

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

paladox opened this issue Jan 17, 2016 · 24 comments
Labels

Comments

@paladox
Copy link
Contributor

paladox commented Jan 17, 2016

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
Copy link
Contributor Author

paladox commented Jan 17, 2016

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

@vladikoff
Copy link
Member

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

@paladox
Copy link
Contributor Author

paladox commented Jan 17, 2016

@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
Copy link
Contributor Author

paladox commented Jan 17, 2016

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
Copy link

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
Copy link
Contributor Author

paladox commented Jan 18, 2016

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
Copy link
Member

Previously, this was working fine

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

@paladox
Copy link
Contributor Author

paladox commented Jan 18, 2016

I tested with 0.11.3 and it dosent work.

So this is supported just the code needs updating.

@paladox
Copy link
Contributor Author

paladox commented Jan 18, 2016

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

@paladox
Copy link
Contributor Author

paladox commented Jan 19, 2016

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
Copy link

@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.

@vladikoff vladikoff added the bug label Jan 23, 2016
@stefcameron
Copy link

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
Copy link
Member

@stefcameron thanks for investigating, checking ...

@vladikoff
Copy link
Member

@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

> [email protected] 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
Copy link
Member

could be related to jshint/jshint#2841

Different reporters?

@paladox
Copy link
Contributor Author

paladox commented Jan 23, 2016

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

Seems to be a bug then.

@stefcameron
Copy link

@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
Copy link

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

@paladox
Copy link
Contributor Author

paladox commented Jan 23, 2016

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
Copy link

@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 jshint/jshint#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
Copy link

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
Copy link
Member

shama commented Feb 17, 2016

@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
Copy link

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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants