Single line ignore switches #1175
Conversation
| @@ -36,25 +37,33 @@ export class EnableDisableRulesWalker extends SkippableTokenAwareRuleWalker { | |||
| return; | |||
| } | |||
|
|
|||
| // keep track of the beginning of the line so that we can use it for same line switches | |||
| if (scanner.getToken() === ts.SyntaxKind.NewLineTrivia) { | |||
There was a problem hiding this comment.
As opposed to tracking this, you could use:
node.getLineAndCharacterOfPosition in conjunction with scanner.getTokenPos
|
I'll see what I can come up with tonight |
|
Looking good to me @patsissons! Let's see if we can simplify things a little using the |
|
I'm all for optimized clean code 👍 what you're suggesting sounds pretty reasonable. |
…ith the scanner instance so that it can interact with it directly.
patsissons
force-pushed
the
master
branch
2 times, most recently
from
954501f to
f0b1abd
Compare
| // end at the beginning of the next line | ||
| this.enableDisableRuleMap[ruleToAdd].push({ | ||
| isEnabled: !isEnabled, | ||
| position: scanner.getTextPos() + 1, |
There was a problem hiding this comment.
Should this be:
position: this.getStartOfLinePosition(node, startingPosition, 1), ?
Possible that the current version is fine, I just don't understand what it's doing exactly
There was a problem hiding this comment.
The third param is line offset, which defaults to 0, the current line. For current line switches we want to start at the beginning of the current line, so line offset isn't necessary.
There was a problem hiding this comment.
Sorry for the lack of clarity, I was referring to if position: scanner.getTextPos() + 1, should be changed
There was a problem hiding this comment.
Oh sorry, i see what you were trying to say now. Either version should yield the same position. scanner.getTextPos() returns the position of the end of the text currently parsed token, the +1 is just to bump the position to the beginning of the next line. I guess that the current expression may be a little terse, I could add an extra comment for clarity if you like?
There was a problem hiding this comment.
No worries, the comment above is clear enough! Just wanted to check that this was as intended. PR merged!
|
👍 Like this new version, much easier to read and understand! Just had one comment above, looks good to me otherwise |
|
Thoughts on tests to cover the start and stop interval creation? Or is that better captured in a new PR? |
|
As far as tests, I think we should be able to move this over to the newer, more visual testing system. (I.e. the files in |
|
@patsissons Do you want to do the test PR? Or should I try to knock it out? Just want to make sure we don't duplicate work |
|
I probably wouldn't get to it for a few days, so go ahead. Plus you'll definitely have a better understanding on how to improve things than I will. |
This PR supports using ESLint style single line ignore switches by using
tslint:disable-lineandtslint:disable-next-line.This PR resolves #144