Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Single line ignore switches #1175

Merged
merged 5 commits into from
Apr 29, 2016
Merged

Single line ignore switches #1175

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c98f11d
updating tests to support new one line switch comments
patsissons Apr 28, 2016
bb88b32
updating regex and rule map to support -line and -next-line
patsissons Apr 28, 2016
af710b3
track the starting position of each line. provide the switch parser w…
patsissons Apr 28, 2016
23df6ce
handle current line and next line switch comments
patsissons Apr 28, 2016
0aebc61
cleaning up code by using node line, col, position conversion functions.
patsissons Apr 29, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 37 additions & 7 deletions src/enableDisableRules.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,21 +40,30 @@ export class EnableDisableRulesWalker extends SkippableTokenAwareRuleWalker {
scanner.getToken() === ts.SyntaxKind.SingleLineCommentTrivia) {
const commentText = scanner.getTokenText();
const startPosition = scanner.getTokenPos();
this.handlePossibleTslintSwitch(commentText, startPosition);
this.handlePossibleTslintSwitch(commentText, startPosition, node, scanner);
}
});
}

private handlePossibleTslintSwitch(commentText: string, startingPosition: number) {
private getStartOfLinePosition(node: ts.SourceFile, position: number, lineOffset = 0) {
return node.getPositionOfLineAndCharacter(
node.getLineAndCharacterOfPosition(position).line + lineOffset, 0
);
}

private handlePossibleTslintSwitch(commentText: string, startingPosition: number, node: ts.SourceFile, scanner: ts.Scanner) {
// regex is: start of string followed by "/*" or "//" followed by any amount of whitespace followed by "tslint:"
if (commentText.match(/^(\/\*|\/\/)\s*tslint:/)) {
const commentTextParts = commentText.split(":");
// regex is: start of string followed by either "enable" or "disable"
// followed optionally by -line or -next-line
// followed by either whitespace or end of string
const enableOrDisableMatch = commentTextParts[1].match(/^(enable|disable)(\s|$)/);
const enableOrDisableMatch = commentTextParts[1].match(/^(enable|disable)(-(line|next-line))?(\s|$)/);

if (enableOrDisableMatch != null) {
const isEnabled = enableOrDisableMatch[1] === "enable";
const isCurrentLine = enableOrDisableMatch[3] === "line";
const isNextLine = enableOrDisableMatch[3] === "next-line";
let rulesList = ["all"];
if (commentTextParts.length > 2) {
rulesList = commentTextParts[2].split(/\s+/);
Expand All @@ -63,10 +72,31 @@ export class EnableDisableRulesWalker extends SkippableTokenAwareRuleWalker {
if (!(ruleToAdd in this.enableDisableRuleMap)) {
this.enableDisableRuleMap[ruleToAdd] = [];
}
this.enableDisableRuleMap[ruleToAdd].push({
isEnabled: isEnabled,
position: startingPosition,
});
if (isCurrentLine) {
// start at the beginning of the current line
this.enableDisableRuleMap[ruleToAdd].push({
isEnabled: isEnabled,
position: this.getStartOfLinePosition(node, startingPosition),
});
// end at the beginning of the next line
this.enableDisableRuleMap[ruleToAdd].push({
isEnabled: !isEnabled,
position: scanner.getTextPos() + 1,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the lack of clarity, I was referring to if position: scanner.getTextPos() + 1, should be changed

Copy link
Contributor Author

@patsissons patsissons Apr 29, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries, the comment above is clear enough! Just wanted to check that this was as intended. PR merged!

});
} else {
// start at the current position
this.enableDisableRuleMap[ruleToAdd].push({
isEnabled: isEnabled,
position: startingPosition,
});
// end at the beginning of the line following the next line
if (isNextLine) {
this.enableDisableRuleMap[ruleToAdd].push({
isEnabled: !isEnabled,
position: this.getStartOfLinePosition(node, startingPosition, 2),
});
}
}
}
}
}
Expand Down
14 changes: 14 additions & 0 deletions test/files/enabledisable.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,5 +27,19 @@ re = /`/;
var AAAaA = 'test'
// tslint:enable

var AAAaA = 'test' // tslint:disable-line
var AAAaA = 'test' /* tslint:disable-line */

// tslint:disable-next-line:quotemark variable-name
var AAAaA = 'test'
/* tslint:disable-next-line:quotemark variable-name */
var AAAaA = 'test'

// tslint:disable
var AAAaA = 'test' // tslint:enable-line:quotemark
// tslint:enable-next-line:variable-name
var AAAaA = 'test'
// tslint:enable

/* tslint:disable:quotemark */
var s = 'xxx';
7 changes: 6 additions & 1 deletion test/ruleDisableEnableTests.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,9 @@ describe("Enable and Disable Rules", () => {
const expectedFailure5 = quotemarkFailure([10, 13], [10, 19]);
const expectedFailure6 = quotemarkFailure([16, 13], [16, 19]);

const expectedFailure7 = quotemarkFailure([39, 13], [39, 19]);
const expectedFailure8 = variableNameFailure([41, 5], [41, 10]);

const ll = new Linter(relativePath, source, options);
const result = ll.lint();
const parsedResult = JSON.parse(result.output);
Expand All @@ -66,6 +69,8 @@ describe("Enable and Disable Rules", () => {
TestUtils.assertContainsFailure(actualFailures, expectedFailure4);
TestUtils.assertContainsFailure(actualFailures, expectedFailure5);
TestUtils.assertContainsFailure(actualFailures, expectedFailure6);
assert.lengthOf(actualFailures, 6);
TestUtils.assertContainsFailure(actualFailures, expectedFailure7);
TestUtils.assertContainsFailure(actualFailures, expectedFailure8);
assert.lengthOf(actualFailures, 8);
});
});