madskristensen / WebEssentials2013

Visual Studio extension
http://vswebessentials.com
Other
945 stars 253 forks source link

Feature: Showing Applicable Lint/Hint Rule in Error List #1717

Open amisner opened 9 years ago

amisner commented 9 years ago

Hi, does anyone here think it would be feasible to give the applicable rule for a particular jsHint or tsLint message in the Visual Studio Error List? Something like the image below.

I'm guessing this would be a somewhat substantial undertaking, but I feel it would pay dividends down the road since it enables new users to these various lint and hint checkers to immediately identify the correct rule causing the message so that they can change/disable it if they want. As it stands now, one has to intrinsically know the actual name of the rule that is generating this message and if they don't know the name they have to scan the config file and guess at which rule is the correct one or do a search online to give them insight into what the correct rule is.

If we can provide the name of the rule causing the lint/hint message right there in the Error List, it saves time tracking that rule down in the config file. Granted not everyone cares to change the rules in these config files, but for those that do this can be a real time saver.

I'd be willing to do a pull request to help implement the feature if someone could point me in the right direction. I'm hoping that the code that checks source files for a particular rule somehow has the ability to access the name of that rule during the check. If that's the case, then implementing this would be rather simple, just update the message to include the rule name, but if the rule name is not available, then I fear this would be more trouble than its worth.

I welcome any thoughts or suggestions on this matter.

2014-12-31 10_29_05-trident - microsoft visual studio

am11 commented 9 years ago

This sounds really interesting to me. Since WE is consuming these upstream (node.js-based) services and do not intervene with their internals, it would probably make sense to split this issue and open it in corresponding repositories:

If these services emit the output with such an info, it will be shown in VS' Error list (as WE just add the message string to the list and have no idea / context of what it says).

It may take some time, but eventually it could happen: (in my experience) at least node-jscs folks welcome and entertain such ideas wholeheartedly. :)

amisner commented 9 years ago

Excellent. Makes sense. It hadn't occurred to me that WE was just outputting what the various linters and hinters were reporting without any translation. Well then, I suppose I will create a feature request issue on the jshint, node-jscs, and tslint githubs. Thanks for that info @am11 !

UPDATE:

am11 commented 9 years ago

That is a great that two of them are already supporting it. :+1:

For tslint, we would need to replace this line with the following:

Message: "TsLint: " + result.getFailure() + " (" + result.getRuleName() + ")",
// wish we had used single quotes in JS from day 1

You can compare our custom reporter function with their example one (they call it custom formatter, other linters call it custom reporter).

Also, we have another guy in this league: CoffeeLint.

amisner commented 9 years ago

Awesome, this sounds promising.

Well this is the best issue item I could find for CoffeLint: https://github.com/clutchski/coffeelint/issues/160 it doesn't say anything about how to access the rule name though.

So what all needs to be done to get these rule names printed out? Is it just a matter of calling "result.getRuleName()" (or the equivalent) method from within each srv-xxx.js file's reporter.prototype.format function?

am11 commented 9 years ago

It will only be applied to the linter-like services. Each linter package has its own object passed to the custom reporter/formatter function. tslint calls it getRuleName(), others may use different name when they receive the functionality.

Note we have three types of node.js based services in there:

(ruby-sass service is not node.js based. it actually calls an exe file)

am11 commented 9 years ago

Turned out, CoffeeLint also emits it: https://github.com/clutchski/coffeelint/issues/367 as .rule. :) So in our custom reporter srv-coffeelint.js, we would need to change this to:

Message: "CoffeeLint: " + error.message + " (" + error.rule + ")",
amisner commented 9 years ago

I see. Makes sense.

Ok, so we know which line needs to be changed for tslint and coffeelint, now all we need to know are the other two, jshint and jscs right?

am11 commented 9 years ago

Yes pretty much. I think node-jscs also emits this information to the reporter. Currently WE node-jscs reporter is broken. I fixed few bits locally, but the vow (promise) is somehow throwing and swallowing the exception message. Need to dig deeper what is going wrong there.

jshint, however certainly does not emit the voilated rule name in the error JSON, for instance:

 {
  "file": "C:\\temp\\App.js",
  "error": {
    "id": "(error)",
    "raw": "Don't make functions within a loop.",
    "code": "W083",
    "evidence": "                    });",
    "line": 421,
    "character": 22,
    "scope": "(main)",
    "reason": "Don't make functions within a loop."
  }
}

Note that the code is something which does not relate to the rule name in their config file (.jshintrc).

amisner commented 9 years ago

Hmm, yes it seems that jshint is pretty limited in its options (http://jshint.com/docs/), in this case the code and raw or reason properties are about the best that could be made available as far as reporting the offending rule. Especially since not all rules are able to be set in the .jshintrc config file.