Start linting for double spaces#20820
Conversation
weswigham
left a comment
There was a problem hiding this comment.
Seems ok, some minor comments.
| } | ||
| // Allow common uses of double spaces | ||
| // * To align `=` or `!=` signs | ||
| // * To alling comments at the end of lines |
| } | ||
|
|
||
| function countDoubleSpaces(line: string): number { | ||
| let count = 0; |
There was a problem hiding this comment.
Why not just (line.match(/\s\s+/g) || []).length?
aeab99b to
1b43adf
Compare
| idx++; | ||
| count++; | ||
| } | ||
| return (line.match(/\s\s/g) || []).length; |
There was a problem hiding this comment.
This matches on a b (4 spaces between the letters, since GH compacts it) twice. Is that intentional?
There was a problem hiding this comment.
The goal of this function is to ignore the rule on uses of multiple spaces that are probably not just accidentally pressing the space bar twice -- we could eliminate this entirely in a followup PR, but if we change to /\s\s+/g we would get some new errors right now.
There was a problem hiding this comment.
There are multiple ways we might want to make this rule stricter, such as not allowing aligning = outside of enums (or even within them).
There was a problem hiding this comment.
Is the heuristic here supposed to be the length of a run of whitespace (for marking that specific run as an error), or the number of individual >1 length whitespace runs on the line (for marking the all instances on the line as an error)? As is, it's half the total length of each >1 length run of whitespace on the line, which seems incorrect, since it's neither of those.
There was a problem hiding this comment.
I've fixed the remaining cases, so we don't need this function any more.
| // the original grammar) by making sure that if we see an ObjectCreationExpression | ||
| // we always consume arguments if they are there. So we treat "new Foo()" as an | ||
| // object creation only, and not at all as an invocation) Another way to think | ||
| // object creation only, and not at all as an invocation). Another way to think |
There was a problem hiding this comment.
the intent was probably for the ) to be a . since the wordy author in the parser also seems to use double spaces after periods, and there's no open parenthesis.
I think @sandersn mentioned before that he expected the linter to catch double spaces in code. A rule outright banning that would result in a lot of lint failures, so I've added exceptions for our frequent alignment of
=signs and other common uses of double spaces.