Rule definitions are very hard to keep consistent with tslint #17

Closed
alexkrauss opened this Issue Oct 4, 2015 · 6 comments

Projects

None yet

4 participants

@alexkrauss
Contributor

While trying to update the rule set to newer tslint and applying it for a realistic project, we realized that this is a nightmare in two dimensions:

  • TsLint is a moving target. There are many new rules, many pending suggestions, and options for the rules keep changing. Thus, any version of the Sonar plugin would be tightly coupled to some specific version of tslint. (One could try to bundle tslint with the plugin, but this is not easy as well).
  • Tslint has no consistent model for its rule options. Most options are just flags, others are values, a few are structured objects. The current mapping code from Sonar rules to tslint.json is incomplete in some important ways, but it's pretty much unfixable at the moment, without going into various special cases for some rules. The tslint project is somewhat aware of this issue (palantir/tslint#629), but I'm not sure when this will improve, as it will certainly break existing tslint config files.

IMHO, to make this work reliably, we should avoid trying to understand the complete rule metamodel of tslint, and just pass in a pre-existing tslint file (which all projects will have anyway). Ideally, SonarQube would let us generate dynamic rules based on the issues encountered. But the API does not support this, so I guess we must have at least the set of rules pre-configured, but not all the options.

So, to make this work reliably, I'd propose to simplify it:

  • The plugin no longer knows about all the rules' options, but just the rule keys and user-readable names. So, rule options cannot be configured in Sonar, but only in tslint.json. On the other hand, we have full expressivity there.
  • The plugin takes the path to a tslint.json file as an option. This configuration file will be used for the analysis. It can be passed at analysis time, so this should be flexible enough.
  • Then, all encountered issues are filtered with the set of rules that are activated and then added to sonar. If any issues are encountered for a rule unknown to the plugin, they can be filed with a generic rule "tslint issue".

I think this will make the plugin simpler and much more robust.

What do you think?

@Pablissimo
Owner

Short answer
Go for it. However, so that it continues to work (ish) out of the box, I'd probably suggest having a fallback default tslint.json with some vaguely sensible default ruleset.

Longer answer
Yeah, the structure of the file's problematic and I think this whole situation's probably the sort of thing that the Sonar guys avoid with their core plugins by not taking dependencies on external apps. Ideally, we'd have a TypeScript parser and linter built into the plugin (same as how the JS one works) and not depend on TsLint at all but keeping up to date with TS becomes the new problem.

What I tried to do was make the plugin feel like the Javascript one in configuration terms:

  • Same way of configuring rules through the project level interface, trying to hide the tslint.json that needs generated to make that work
  • Same naming conventions for parameters like LCOV source

However as an experiment I think you're right that it's not gone too well, and a lack of control over the TsLint version in use means the whole thing's a moveable feast. I think the C# plugin (the only other 'core' one I can think of that has external dependencies) gets away with it a bit easier as FxCop's a slower-moving target but don't know if there's anything else to learn from how they deal with this.

@alexkrauss
Contributor

Yeah, the structure of the file's problematic and I think this whole situation's probably the sort of thing that the Sonar guys avoid with their core plugins by not taking dependencies on external apps. Ideally, we'd have a TypeScript parser and linter built into the plugin (same as how the JS one works) and not depend on TsLint at all but keeping up to date with TS becomes the new problem.

I understand the reasoning why SonarQube folks try to do their own thing instead of depending on other projects, but I don't think this approach can work for more than a few major languages. Until typescript has become becomes the undisputed lingua franca for the web, noone is going to write and maintain a decent linter that is tightly coupled to Sonar. Instead, integrating standard stuff is what works.

Working on that... will come with a PR soon, once we got this running.

@benediktarnold

Any new on your PR? I stumbled upon this issue while changing the no-trailling-comma rule to trailing-comma rule. This was an easy task, but it is only the tip of the iceberg ;-)

@tsjensen
tsjensen commented Mar 9, 2016

Great idea! Looking forward to this PR also ;-)

@Pablissimo
Owner

Just for giggles and after the best part of a bottle of a particularly cheeky Cabernet Sauvignon I had a bash at this in the issue17 branch. It's in need of unit testing (and more interactive testing) but what I have aimed to do is as follows:

  • No longer generate tslint.json from SonarQube configuration, instead a new key sonar.ts.tslintconfigpath can be specified in your sonar-project.properties or in the admin interface (defaults to tslint.json next to sonar-project.properties)
  • Updated the default ruleset to match current trunk of tslint (as of 09/03/16)
  • Removed all parameters for all the rules, so that they're just on or off
  • Mapped all unknown results from tslint to a new rule, 'tslint-issue' that becomes a bucket for all rule breaches the plugin doesn't know about

It'll be a breaking change for any project using it as I've not yet added a default to use in the event a file isn't specified, but if you fancy cloning, building and having a bash we can see if it covers the bases you're after.

Would probably take a tslint.json from the sample on the tslint repo and then turn on/off the options you want, as the -i command-line option of tslint appears to be... not quite right.

@Pablissimo Pablissimo self-assigned this Mar 10, 2016
@Pablissimo
Owner

Candidate release available with the above changes, not yet merged into trunk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment