Skip to content
This repository has been archived by the owner on Feb 21, 2022. It is now read-only.

Commit

Permalink
[bug] fixed brace-style rule (closes #93)
Browse files Browse the repository at this point in the history
  • Loading branch information
hadalin committed Aug 29, 2016
1 parent 8ba8d47 commit 874440f
Show file tree
Hide file tree
Showing 2 changed files with 229 additions and 30 deletions.
44 changes: 34 additions & 10 deletions src/rules/braceStyleRule.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,22 +49,35 @@ class BraceStyleWalker extends Lint.RuleWalker {
this.allowSingleLine = this.getOptions()[1] && this.getOptions()[1].allowSingleLine;
}

// check that the "catch" keyword is on the correct line.
// check that the "catch" and "finally" keyword are on the correct line.
// all other checks regarding try/catch statements will be covered in the "visitBlock" callback
protected visitTryStatement(tryStatement: ts.TryStatement): void {
super.visitTryStatement(tryStatement);

const catchClause = tryStatement.getChildren().filter(ch => ch.kind === ts.SyntaxKind.CatchClause).shift();
const previousNode = tryStatement.getChildren()[tryStatement.getChildren().indexOf(catchClause) - 1];
const openingBracketError = this.areOnSameLine(previousNode, catchClause) !== (this.braceStyle === BraceStyle.OneTBS);
const checkTryStatementError = (node: ts.Node): void => {
const previousNode = this.getPreviousNode(tryStatement.getChildren(), node);
const openingBracketError = this.areOnSameLine(previousNode, node) !== (this.braceStyle === BraceStyle.OneTBS);

if (this.allowSingleLine && this.getStartPosition(catchClause).line === this.getEndPosition(tryStatement).line) {
return;
if (this.allowSingleLine && this.getStartPosition(node).line === this.getEndPosition(tryStatement).line) {
return;
}

if (openingBracketError) {
const failureString = this.braceStyle === BraceStyle.OneTBS ? Rule.FAILURE_STRING.open : Rule.FAILURE_STRING.openAllman;
this.addFailure(this.createFailure(node.getStart(), node.getWidth(), failureString));
}
};

// check catch
const catchClause = tryStatement.catchClause;
if (catchClause) {
checkTryStatementError(catchClause);
}

if (openingBracketError) {
const failureString = this.braceStyle === BraceStyle.OneTBS ? Rule.FAILURE_STRING.open : Rule.FAILURE_STRING.openAllman;
this.addFailure(this.createFailure(catchClause.getStart(), catchClause.getWidth(), failureString));
// check finally
const finallyBlock = tryStatement.finallyBlock;
if (finallyBlock) {
checkTryStatementError(finallyBlock);
}
}

Expand All @@ -86,7 +99,7 @@ class BraceStyleWalker extends Lint.RuleWalker {
return;
}

// if the if statement doesn't have a "block" element, it means it has no braces,
// if the if statement doesn't have a "block" element, it means it has no braces,
// and when there are no braces, there are no problems
if (!ifStatement.getChildren().some(ch => ch.kind === ts.SyntaxKind.Block)) {
return;
Expand Down Expand Up @@ -148,4 +161,15 @@ class BraceStyleWalker extends Lint.RuleWalker {
private getEndPosition(node: ts.Node) {
return node.getSourceFile().getLineAndCharacterOfPosition(node.getEnd());
}

// returns previous node which is either block or catch clause (no keywords, etc).
private getPreviousNode(children: ts.Node[], node: ts.Node): ts.Node {
let position = children.indexOf(node) - 1;
while (position >= 0) { // is first child always block or catch clause?
if (children[position].kind === ts.SyntaxKind.Block || children[position].kind === ts.SyntaxKind.CatchClause) {
return children[position];
}
position -= 1;
}
}
}
215 changes: 195 additions & 20 deletions src/test/rules/braceStyleRuleTests.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,28 @@ const scripts = {
handleError();
}`,

`try {
somethingRisky();
} catch(e) {
handleError();
} finally() {
doSomething();
}`,

`try {
somethingRisky();
} finally() {
doSomething();
} catch(e) {
handleError();
}`,

`try {
somethingRisky();
} finally() {
doSomething();
}`,

// when there are no braces, there are no problems
`if (foo) bar();
else if (baz) boom();`
Expand All @@ -48,6 +70,26 @@ const scripts = {
handleError();
}`,

`try
{
somethingRisky();
} catch(e)
{
handleError();
}
} finally(e)
{
doSomething();
}`,

`try
{
somethingRisky();
} finally(e)
{
doSomething();
}`,

`if (foo) {
bar();
}
Expand All @@ -56,7 +98,7 @@ const scripts = {
}`,

`if (foo) {
bar();
bar();
} else { baz(); }`
]
},
Expand Down Expand Up @@ -84,6 +126,24 @@ const scripts = {
handleError();
}`,

`try {
somethingRisky();
}
catch(e) {
handleError();
}
finally() {
doSomething();
}
`,

`try {
somethingRisky();
}
finally {
doSomething();
}`,

// when there are no braces, there are no problems
`if (foo) bar();
else if (baz) boom();`
Expand All @@ -107,6 +167,27 @@ const scripts = {
handleError();
}`,

`try
{
somethingRisky();
} catch(e)
{
handleError();
} finally()
{
doSomething();
}
`,

`try
{
somethingRisky();
} finally()
{
doSomething();
}
`,

`if (foo) {
bar();
} else {
Expand Down Expand Up @@ -144,6 +225,30 @@ const scripts = {
handleError();
}`,

`try
{
somethingRisky();
}
catch(e)
{
handleError();
}
finally()
{
doSomething();
}
`,

`try
{
somethingRisky();
}
finally()
{
doSomething();
}
`,

// when there are no braces, there are no problems
`if (foo) bar();
else if (baz) boom();`
Expand All @@ -165,6 +270,25 @@ const scripts = {
handleError();
}`,

`try {
somethingRisky();
} catch(e)
{
handleError();
} finally()
{
doSomething();
}
`,

`try {
somethingRisky();
} finally()
{
doSomething();
}
`,

`if (foo) {
bar();
} else {
Expand All @@ -183,13 +307,26 @@ const scripts = {

`try { somethingRisky(); } catch(e) { handleError(); }`,

`if (foo) {
bar();
`try { somethingRisky(); } catch(e) { handleError(); } finally() { doSomething(); }`,

`try { somethingRisky(); } finally(e) { doSomething(); }`,

`if (foo) {
bar();
} else { baz(); }`,

`try {
`try {
foo();
} catch(e) { bar(); }`,

`try {
foo();
} catch(e) { bar(); }`
} catch(e) { bar(); }
} finally() { doSomething(); }`,

`try {
foo();
} finally() { doSomething(); }`
]
},
stroustrup: {
Expand All @@ -204,15 +341,33 @@ const scripts = {
`try { somethingRisky(); }
catch(e) { handleError(); }`,

`if (foo) {
`try { somethingRisky(); }
catch(e) { handleError(); }
finally() { doSomething(); }`,

`try { somethingRisky(); }
finally() { doSomething(); }`,

`if (foo) {
bar();
}
}
else { baz(); }`,

`try {
`try {
foo();
}
catch(e) { bar(); }`,

`try {
foo();
}
catch(e) { bar(); }`
}
catch(e) { bar(); }
finally() { doSomething(); }`,

`try {
foo();
}
finally() { doSomething(); }`
]
},
allman: {
Expand All @@ -227,16 +382,36 @@ const scripts = {
`try { somethingRisky(); }
catch(e) { handleError(); }`,

`if (foo)
{
`try { somethingRisky(); }
catch(e) { handleError(); },
finally() { doSomething(); }`,

`try { somethingRisky(); }
finally(e) { doSomething(); }`,

`if (foo)
{
bar();
} else { baz(); }`,

`try
{
`try
{
foo();
}
catch(e) { bar(); }`,

`try
{
foo();
}
catch(e) { bar(); }
finally() { doSomething(); }`,

`try
{
foo();
}
catch(e) { bar(); }`
}
finally() { doSomething(); }`
]
}
}
Expand All @@ -246,8 +421,8 @@ describe(rule, function test() {
const onetbsConfig = { rules: { 'brace-style': [true, '1tbs'] } };
const stroustrupConfig = { rules: { 'brace-style': [true, 'stroustrup'] } };
const allmanConfig = { rules: { 'brace-style': [true, 'allman'] } };
const onetbsConfigWithException = { rules: { 'brace-style': [true, 'stroustrup', { allowSingleLine: true }] } };
const stroustrupConfigWithException = { rules: { 'brace-style': [true, '1tbs', { allowSingleLine: true }] } };
const onetbsConfigWithException = { rules: { 'brace-style': [true, '1tbs', { allowSingleLine: true }] } };
const stroustrupConfigWithException = { rules: { 'brace-style': [true, 'stroustrup', { allowSingleLine: true }] } };
const allmanConfigWithException = { rules: { 'brace-style': [true, 'allman', { allowSingleLine: true }] } };

it('should pass when "1tbs"', function testVariables() {
Expand Down Expand Up @@ -286,8 +461,8 @@ describe(rule, function test() {
makeTest(rule, scripts.allowSingleLine.allman.valid, true, allmanConfigWithException);
});

// all scripts in the "allowSingleLine" object should *only* pass if
// allowSingleLine === true, so let's check to make sure they
// all scripts in the "allowSingleLine" object should *only* pass if
// allowSingleLine === true, so let's check to make sure they
// fail if allowSingleLine === false
it('should fail when "1tbs"', function testVariables() {
makeTest(rule, scripts.allowSingleLine.onetbs.valid, false, onetbsConfig);
Expand Down

0 comments on commit 874440f

Please sign in to comment.