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

Added unnecessary-else Rule #4502

Merged
merged 20 commits into from
Mar 2, 2019

Conversation

debsmita1
Copy link
Contributor

@debsmita1 debsmita1 commented Feb 4, 2019

PR checklist

Overview of change:

This Rule disallows else blocks following if blocks ending with a return , throw, break or continuestatement.

Is there anything you'd like reviewers to focus on?

This is my very first open-source contribution, so all kinds of feedback is appreciated.

CHANGELOG.md entry:

[new-rule] unnecessary-else

@palantirtech
Copy link
Member

Thanks for your interest in palantir/tslint, @debsmita1! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

* Added more testcases
* Made minor changes to the FAILURE_STRING
Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Great start! Some architectural changes should be made.

src/configs/recommended.ts Outdated Show resolved Hide resolved
src/rules/noUnnecessaryElseRule.ts Outdated Show resolved Hide resolved
src/rules/noUnnecessaryElseRule.ts Outdated Show resolved Hide resolved
test/rules/no-unnecessary-else/test.ts.lint Outdated Show resolved Hide resolved
src/rules/noUnnecessaryElseRule.ts Outdated Show resolved Hide resolved
src/rules/noUnnecessaryElseRule.ts Outdated Show resolved Hide resolved
src/rules/noUnnecessaryElseRule.ts Outdated Show resolved Hide resolved
src/rules/noUnnecessaryElseRule.ts Outdated Show resolved Hide resolved
test/rules/no-unnecessary-else/test.ts.lint Outdated Show resolved Hide resolved
test/rules/no-unnecessary-else/test.ts.lint Outdated Show resolved Hide resolved
@JoshuaKGoldberg JoshuaKGoldberg changed the title Added no-unnecessary-else Rule Added unnecessary-else Rule Feb 16, 2019
@debsmita1
Copy link
Contributor Author

debsmita1 commented Feb 17, 2019

I have addressed all your comments.
screenshot from 2019-02-17 18-42-01
Cant figure out why these 2 are failing. Could you please review it again @JoshuaKGoldberg

Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Looks close, thanks for the docs & code examples! Just waiting on simplifying the recursive calls and adding handling for non-block if bodies.

src/rules/unnecessaryElseRule.ts Outdated Show resolved Hide resolved
}
ts.forEachChild(node.thenStatement, cb);
} else {
return ts.forEachChild(node, cb);
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like you have a few different ts.forEachChilds: individual ones for the node's elseStatement and thenStatement if it's an IfStatement, but just a regular recursion otherwise? You should be able to simplify this a bit by only having one call to ts.forEachChild that's always called.

Also, if jumpStatement is undefined but node.elseStatement isn't, this code will skip it. That seems like a potential bug?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done be2f8b4

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry @debsmita1 I think I wasn't very clear here. I'm suggesting you don't need to have ts.forEachChild statements in the big if (utils.isIfStatement block:

if (utils.isIfStatement(node)) {
    // all your glorious rule logic here
    // no ts.forEachChild in this area
}

ts.forEachChild(node, cb);

Note the lack of else statement: a single ts.forEachChild should recurse on all the node's children no matter what. Could you please switch to that? It's a bit less code and better practice to minimize recursive calls (since they can get tricky in larger rules).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done f36d420

Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

This is looking great! A few simplifications to the internals and error message to go, I think.

Also, about the build failures: the build is running linting with this rule enabled and finding failures in TSLint's code base. So it looks like the rule is working! 😄 You'll need to fix those failures to get the build to pass.

}
ts.forEachChild(node.thenStatement, cb);
} else {
return ts.forEachChild(node, cb);
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry @debsmita1 I think I wasn't very clear here. I'm suggesting you don't need to have ts.forEachChild statements in the big if (utils.isIfStatement block:

if (utils.isIfStatement(node)) {
    // all your glorious rule logic here
    // no ts.forEachChild in this area
}

ts.forEachChild(node, cb);

Note the lack of else statement: a single ts.forEachChild should recurse on all the node's children no matter what. Could you please switch to that? It's a bit less code and better practice to minimize recursive calls (since they can get tricky in larger rules).

src/rules/code-examples/unnecessaryElse.examples.ts Outdated Show resolved Hide resolved
src/rules/unnecessaryElseRule.ts Outdated Show resolved Hide resolved
src/rules/unnecessaryElseRule.ts Outdated Show resolved Hide resolved
@JoshuaKGoldberg
Copy link
Contributor

screenshot from 2019-02-17 18-42-01

I also can't figure out why those are failing at first glance. Very spooky. If they're still failing after the introduced lint complaints are fixed, we can take a deeper look...

Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Whoohoo, this looks great! Thanks for sticking through review @debsmita1; very exciting to have this rule in! 🙌

(additional thanks for fixing all the complaints it found across existing source files...!)

@JoshuaKGoldberg JoshuaKGoldberg merged commit 88916a2 into palantir:master Mar 2, 2019
Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

I find this rule pretty opinionated, sometimes impairing readability, and I don't want to enable this in the tslint codebase. Sorry I didn't get around to look at this before it got merged, but I think we should disabled this for tslint's own code in tslint.json at the root of this repo, and I would not add it to tslint:recommended or tslint:latest.

it is unnecessary to add an \`else\` statement.
The contents that would be in the \`else\` block can be placed after the end of the \`if\` block.`,
ruleName: "unnecessary-else",
type: "functionality",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a purely stylistic rule; is there any functional difference?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rule suggestion: unnecessary-else
4 participants