Cleaner Configuration of Rules

[jkillian]

There are currently a few different possible levels of complexity of options for rules:

1. enabled/disabled only (ex: no-debugger)
2. flag options (ex: check-whitespace, each option indicates whether something should be checked or not)
3. fixed length argument lists (ex: max-line-length)
4. variable length argument lists (ex: ban)
5. key/value pairs, nested lists and key/value pairs, etc. (no current rules)

The current Boolean/Array paradigm of declaring options only supports the first two of this optimally. It also is functional for the third option, but doesn't really model it well. It's not ideal at all for the fourth case.

There are more and more feature requests which require a more descriptive way to list options (#562 and #522), and it makes sense to have a system that better facilitates rules like this.

Any thoughts on how we should do this?

[alexkrauss]

Most importantly, any refactoring here should make rule arguments statically typed in some way. That is, a rule should declare which arguments it knows about, and what type they have.

With the current model, it is pretty much impossible to implement, e.g., a GUI that allows configuration of tslint rules.

[jkillian]

@vvakame wondering if you have any feedback regarding this? I know tsfmt provides an option to read from tslint.json

[vvakame]

You do not have to care! I'll support new tslint.json.

[jkillian]

I think something similar to how ESLint does thing might work well. Here's an example of how a rule declares its configuration schema.

[jkillian]

Proposal for new config schema (taken from #345):

I'm thinking about implementing this soon, I like the idea of each rule taking an object as configuration. (This is also very nice code-wise, since rules only accept a boolean or array right now, we can implement this feature with no backwards-compat breakage.)

I like a format similar to what's specified above:

"rules": {
  "align": {
    "mode": "off",
    "options": ["parameters", "arguments"]
  }
}

"mode": could be either "off", "warn", "error", or "on". If "on", the default level of warning would be used.

Open questions:

• Should we stick with the ESLint way of things and also allow 0, 1, 2, and 3 instead of "off", "warn", "error", "on", etc?
• Do we want an "on" option at all?
• I like the name "mode" because it's easy to type, but should we pick a different name?

I also like how this proposal makes it easy to add in additional options for things like auto-fixing, as mentioned above.

This would probable be implemented in two steps:

1. Adding in the code to handle object configurations for rules.
2. Writing the code to make different modes act differently (exit status, etc)

[jkillian]

I'm fairly sure the number severities in ESLint are backwards compatibility only. Unless you use the same rule format (eg an array rather than an object) there's not much point I would say.
Perhaps "level" instead of mode?

Agreed. The number system is pretty unclear at first glance, let's not go down that road with TSLint

[Lexicality]

I'm still not entirely sure what "on" would be for? Is it an alias to "error" or is it entirely dependent on the rule?

[jkillian]

The idea was that in the future you could set a default level, and anything that was "on" would use that level. Not 100% sure if it's a good idea or not yet.

[olore]

I started work on this. Interested in any feedback. I am working over here - https://github.com/olore/tslint/tree/enhanced-config-options

{
  "rules": {
    "max-line-length": {
      "mode": "warn",
      "options": 10
    },
    "no-eval": {
      "mode": "error"
    },
    "interface-name": true
}

I have a few questions -

1. I plan on maintaining backward compatibility with the current tslint.json format. Is that a worthwhile endeavor? Or can we just rip the bandaid off? Looks like I will miss 4.0, so maybe support both in 4.x and switch completely 5.0? Or maybe we keep backward compat - in the example above, it's nice to have "interface-name": true all on one line. Thoughts?
2. Should I concern myself with backward compatibility with the formatters?
   1. In order to maintain the interface of format(failures: RuleFailure[], fixes?: RuleFailure[]) I added a 3rd optional param, which I don't love. Open to suggestions here. The proseFormatter seems to be the only one that cares about fixes

[nchen63]

I think we should consider not putting the mode inside of each rule, but have blocks of rules with their own custom configurations. Since there's a need for exclude globs (#73) and we now have jsRules inside tslint.json, I think it makes sense to combine these two concepts so that people can write their own sets of rules with each set having its own configuration. For example:

{
  "rule-sets": {
    "my-js-rules": {
       "include": ["**/*.{js,jsx}"],
       "level": "error",
       "rules": {
           "rule-one": true,
           "rule-two": true
       }
    },
    "my-warning-rules": {
       "include": ["**/*.{ts,tsx}"],
       "exclude": ["./build", "*.d.ts"],
       "level": "warning",
       "rules": {
           "rule-one": true,
           "rule-two": true
       }
    },
    "my-error-rules": {
       "include": ["**/*.{ts,tsx}"],
       "exclude": ["./build", "*.d.ts"],
       "level": "error",
       "rules": {
           "rule-three": true,
           "rule-four": true
       }
    }
  }
}

It's easier to move rules between these rule sets than changing "warning" to "error", for each rule you want to change. You can configure any number of these custom rule sets that can apply different rules to different sets of files. Plus it's easily backwards compatible since nothing inside rules changes so parsing stays the same.

[olore]

@nchen63, thanks for the thoughts. I like the ability to group the rules by level, I'll see how I can incorporate that somehow. It's definitely repetitive right now.

After taking a peek at ESLint and reading through #73, namely @jkillian's #73 (comment) am in the camp that thinks tslint.json should only define the how, not the what to lint.

#73 doesn't convince me of the importance of implementing an ignore/globbing feature into tslint.json.

[adidahiya]

I am in the camp that thinks tslint.json should only define the how, not the what to lint.

+1, I am also in this camp

[joelday]

@jkillian The lack of severity support is adds difficulty to advocating for TypeScript within my teams. Thank you!