Integrate no keywords rule into variable-name rule

[jkillian]

@myitcv:

@adidahiya and I spoke offline and he suggested merging your rule into the variable-name rule.

Basically, we want to have all functionality related to the names of variables in one rule. Two benefits to this: first, it makes it easier to find the rules/options you're looking for when related functionality is grouped together. Second, it is a little bit better performance-wise, as it's one less rule to traverse the AST.

I've made a PR with the modifications, does it look good to you?

[adidahiya]

don't use tabs

[myitcv]

@jkillian

Basically, we want to have all functionality related to the names of variables in one rule

No fundamental objection, and sounds like a reasonable goal. See my comment at the end of this reply.

Two benefits to this: first, it makes it easier to find the rules/options you're looking for when related functionality is grouped together.

Agreed. However I will add that it might be worthwhile (in the near future) having a 'registry' of rules, where you can tag each rule with certain keywords (no pun intended), easily navigate to a description of the rule and some examples (examples that are compiled examples so they don't fall out of date) etc. This would obviate the need to group things into the same rule. But I believe your point still stands in this case.

Second, it is a little bit better performance-wise, as it's one less rule to traverse the AST.

Point taken, although you could say this about every rule.... There are other ways to solve that performance problem.

All that said... The only reason I made this a separate rule in the first place was because what's being enforced here feels like something the compiler should be doing in any case. That's a subjective comment I know, and folks in the TypeScript core team can and do think that things like this should be linter rules (root issue). The naming convention for a variable however is more subjective and is definitely not (if you assume the syntax is correct) something the compiler would ever fail on.

Equally, however, it's just as easy to remove an option from the variable-name rule so maybe it's six of one, half a dozen the other.

But hopefully you understand my rationale for making it a separate rule in the first place.

[jkillian]

Yep, I think either rule or rule option works fine in the end.

Just curious, have you ever run into performance issues in general? If people are, then it's something to think about, perhaps in conjunction with #680

[myitcv]

Just curious, have you ever run into performance issues in general

Not as yet, but our app is not massive. And importantly the per file lint runtime is relatively low, meaning it's quite acceptable to have a separate terminal showing you the lint errors on save (for example)

If people are, then it's something to think about, perhaps in conjunction with #680

Indeed, and it's at this point we will probably need to consider an architecture whereby the linter can operate in a 'watch' mode, a la tsc --watch. Because otherwise per-file lint times start to go through the roof. And if we were to do this we start getting very close to TypeScript #3003

[adidahiya]

👍 lgtm

[alexeagle]

👍 (though technically these are not keywords)
looking forward to the next release

[myitcv]

though technically these are not keywords

Agreed, not all of them are keywords, but I borrowed from the spec for the best possible all-encompassing term that captures the spirit of what we're after here. number is a keyword according to the language of the spec, with Number its primitive type counterpart. Very open to other suggestions.

Incidentally, I think we should add Undefined to this list (even though it cannot be referenced as a type)

[jkillian]

PR to add Undefined for consistency: #749

[adidahiya]

@alexeagle @myitcv just published a dev release, 3.0.0-dev.1, with this new rule option

[myitcv]

@adidahiya great thanks