Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update: for yoda, add a fixer #7199

Merged
merged 1 commit into from Sep 29, 2016
Merged

Update: for yoda, add a fixer #7199

merged 1 commit into from Sep 29, 2016

Conversation

not-an-aardvark
Copy link
Member

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[x] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

Please check each item to ensure your pull request is ready:

  • I've read the pull request guide
  • I've included tests for my change
  • I've updated documentation for my change (if appropriate)

What changes did you make? (Give an overview)

This adds a fixer for yoda. When yoda reports that a binary expression should be flipped, the fixer can flip the sides of the expression, along with the operator, to fix the issue.

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

In general, flipping around the order of a comparison is unsafe, because it changes execution order (e.g. in if (foo() === bar()), foo() gets called before bar()). However, I don't think this is a problem for yoda because one side of the expression is always a literal, so it won't have side-effects.

@mention-bot
Copy link

@not-an-aardvark, thanks for your PR! By analyzing the annotation information on this pull request, we identified @vitorbal, @platinumazure and @puzrin to be potential reviewers

@eslintbot
Copy link

LGTM

platinumazure
platinumazure requested changes Sep 21, 2016
View reviewed changes
Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

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

Besides the one inline change I've noted, could we also see a couple of tests where whitespace between tokens is (a) not present, and (b) inconsistent? Just so we know what to expect in those cases. Thanks!

lib/rules/yoda.js
!(!isEqualityOperator(node.operator) && onlyEquality) &&
isComparisonOperator(node.operator) &&
!(exceptRange && isRangeTest(context.getAncestors().pop()))
) {
context.report({
node,
message: "Expected literal to be on the right side of {{operator}}.",
message: `Expected literal to be on the ${always ? "left" : "right"} side of {{operator}}.`,
Copy link
Member

Choose a reason for hiding this comment

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

In order to facilitate possible i18n someday, could the "left"/"right" interpolation be performed using our double-brace substitution (i.e., same as how "{{operator}}" is handled)? Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I think left/right is part of the message. E.g.: it's 左/右 in Japanese.

@eslintbot
Copy link

LGTM

@not-an-aardvark
Copy link
Member Author

not-an-aardvark commented Sep 21, 2016 •
edited

Updated to add inconsistent/missing spacing tests and use the double-brace substitution.

edit: Updated again to remove the redundant ternary expressions here and replace them with expectedNonLiteral and expectedLiteral, respectively.

@eslintbot
Copy link

LGTM

@platinumazure
Copy link
Member

platinumazure commented Sep 21, 2016 •
edited

Still LGTM even after the last change (removing redundant ternary expressions). Thanks!

@kaicataldo
Copy link
Member

I appreciate the commit message for this one 👍

mysticatea
mysticatea approved these changes Sep 23, 2016
View reviewed changes
Copy link
Member

@mysticatea mysticatea left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@mysticatea mysticatea added enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Sep 23, 2016
@vitorbal
Copy link
Member

i'll champion this!

@vitorbal vitorbal self-assigned this Sep 29, 2016
@vitorbal vitorbal merged commit 89787b2 into eslint:master Sep 29, 2016
@vitorbal
Copy link
Member

Thanks for working on this, @not-an-aardvark! 🎉

@not-an-aardvark not-an-aardvark deleted the yoda-fixer branch September 29, 2016 15:42
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@platinumazure platinumazure platinumazure approved these changes

@mysticatea mysticatea mysticatea approved these changes

Assignees

@vitorbal vitorbal

Labels
archived due to age This issue has been archived; please open a new issue for any further discussion enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@not-an-aardvark @mention-bot @eslintbot @platinumazure @kaicataldo @vitorbal @mysticatea @jquerybot