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

Added no-tautology-expression rule #4470

Merged
merged 11 commits into from
Mar 11, 2019

Conversation

noamyogev84
Copy link
Contributor

@noamyogev84 noamyogev84 commented Jan 19, 2019

PR checklist

Overview of change:

Add a basic rule that validates that two equal literals are not being compared.

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

CHANGELOG.md entry:

[new-rule] no-tautology-expression

@palantirtech
Copy link
Member

Thanks for your interest in palantir/tslint, @noamyogev84! 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.

@JoshuaKGoldberg JoshuaKGoldberg changed the title Issue 803 Added no-tautology-expressions rule Jan 19, 2019
src/rules/noTautologyExpressionRule.ts Outdated Show resolved Hide resolved
src/rules/noTautologyExpressionRule.ts Outdated Show resolved Hide resolved
src/rules/noTautologyExpressionRule.ts Show resolved Hide resolved
@noamyogev84
Copy link
Contributor Author

Good to see you here @JoshuaKGoldberg . 😄
I'll continue to work on this.

@noamyogev84
Copy link
Contributor Author

@JoshuaKGoldberg,
I have these 2 errors when running yarn test:

  1. 1) Executable Files exits with code 0 if correct file is passed:
  2. 2) Executable Files exits with code 0 if several files passed without -f flag:
    tried to follow 2 tests fail on just cloned repo #3443 with no success. Do you have any idea what's the problem?

@JoshuaKGoldberg
Copy link
Contributor

☹ that's unfortunate @noamyogev84. For now, you can always delete those tests locally and/or only run yarn run test:rules, since your changes to add this rule don't affect those tests.

If that doesn't work for you, ping me on Gitter?

@noamyogev84
Copy link
Contributor Author

noamyogev84 commented Jan 29, 2019

Hi @JoshuaKGoldberg, sorry for the long wait. 🙏
Did some rework. Waiting for your review.

As you see, the error with yarn test i had locally, happens in the build server as well. 😢

@noamyogev84
Copy link
Contributor Author

H @JoshuaKGoldberg. Pinging you one more time... 🙏

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.

Good progress! A few things to touch up.

src/rules/noTautologyExpressionRule.ts Outdated Show resolved Hide resolved
test/rules/no-tautology-expression/test.ts.lint Outdated Show resolved Hide resolved
src/rules/noTautologyExpressionRule.ts Outdated Show resolved Hide resolved
src/rules/noTautologyExpressionRule.ts Outdated Show resolved Hide resolved
src/rules/noTautologyExpressionRule.ts Outdated Show resolved Hide resolved
@JoshuaKGoldberg
Copy link
Contributor

Looking through the CircleCI failures, your rule has some kind of logic problem, because it's flagging acceptable binary expressions in TSLint's source as tautologies.

@adidahiya
Copy link
Contributor

friendly ping @noamyogev84, do you expect to have time to return to this PR? thanks

@noamyogev84
Copy link
Contributor Author

noamyogev84 commented Mar 1, 2019

Hi @adidahiya. I'll do some work, this weekend hopefully. 🕵️

@noamyogev84
Copy link
Contributor Author

Hi @JoshuaKGoldberg , @adidahiya .
added fixes and ready for you review. 🍻

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.

Just the (late-breaking, sorry!) change for more primitive types! Otherwise this is looking great!

src/rules/noTautologyExpressionRule.ts Outdated Show resolved Hide resolved
test/rules/no-tautology-expression/test.ts.lint Outdated Show resolved Hide resolved
src/rules/noTautologyExpressionRule.ts Outdated Show resolved Hide resolved
@noamyogev84
Copy link
Contributor Author

I think we're done 😃
@JoshuaKGoldberg

}
} else if (
(isBooleanLiteral(node.left) && isBooleanLiteral(node.right)) ||
(isNullLiteral(node.left) && isNullLiteral(node.right))
Copy link
Contributor

Choose a reason for hiding this comment

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

Continuing the previous conversation: this doesn't capture numeric literals or undefined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

numeric and string literals are covered earlier.
what about undefined? what do you mean by covering this ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, wow, I should review when fully awake. This looks great. I had forgotten undefined is an identifier.

}
} else if (
(isBooleanLiteral(node.left) && isBooleanLiteral(node.right)) ||
(isNullLiteral(node.left) && isNullLiteral(node.right))
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, wow, I should review when fully awake. This looks great. I had forgotten undefined is an identifier.

@JoshuaKGoldberg
Copy link
Contributor

JoshuaKGoldberg commented Mar 11, 2019

@noamyogev84 you've persisted superbly through multiple rounds of code review and slightly inaccurate questioning on my part. Thanks so much for the rule! This is going to be great for catching silly mistakes in user code.

@JoshuaKGoldberg JoshuaKGoldberg merged commit 55160ae into palantir:master Mar 11, 2019
@adidahiya adidahiya changed the title Added no-tautology-expressions rule Added no-tautology-expression rule Mar 12, 2019
@noamyogev84
Copy link
Contributor Author

@JoshuaKGoldberg it's my pleasure man! 🥃

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.

4 participants