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.

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

New: no-mixed-operators rule (fixes #6023) #6241

Merged
merged 1 commit into from Jun 9, 2016
Merged

New: no-mixed-operators rule (fixes #6023) #6241

merged 1 commit into from Jun 9, 2016

Conversation

mysticatea
Copy link
Member

Fixes #6023.
This PR is a reference for discussion.

@mysticatea mysticatea added the do not merge This pull request should not be merged yet label May 22, 2016
@eslintbot
Copy link

LGTM

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @vitorbal, @platinumazure and @michaelficarra to be potential reviewers

jfmengels
jfmengels reviewed Jun 6, 2016
View reviewed changes
docs/rules/no-mixed-operators.md
@@ -0,0 +1,125 @@
# Disallow mixes of different operators (no-mixed-operators)

Enclosing complex expressions by parentheses makes developer's intention clarifying, then the code would get more readability.
Copy link
Contributor

Choose a reason for hiding this comment

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

... clarifies the developer's intention, which makes the code more readable.

@jfmengels
Copy link
Contributor

Thanks for tackling this @mysticatea :)

Presets, as discussed in the original issue could eventually be added later (though I don't know when later would be?). Apart from the ignoreSamePrecedence default value that we could discuss and some typos to fix, I think this is pretty great :)

@mysticatea
Copy link
Member Author

Thank you for many correction!
I'll update those.

@eslintbot
Copy link

LGTM

@michaelficarra
Copy link
Member

michaelficarra commented Jun 7, 2016 •
edited

"ignoreSamePrecedence": false is a double-negative. Maybe "reportSamePrecedence": true instead?

edit: Or how about "allowSamePrecedence": false?

@mysticatea
Copy link
Member Author

I will update it after I'm back to my computer.

@eslintbot
Copy link

LGTM

@mysticatea mysticatea removed the do not merge This pull request should not be merged yet label Jun 8, 2016
@mysticatea
Copy link
Member Author

I updated this with allowSamePrecedence.

@ilyavolodin
Copy link
Member

Would this not conflict with no-extra-parent rule?

@jfmengels
Copy link
Contributor

jfmengels commented Jun 8, 2016 •
edited

@ilyavolodin It could. @mysticatea wrote about it in the rule documentation here https://github.com/eslint/eslint/pull/6241/files#diff-9281570e12d9d9c973cc09aa963063ccR17. This conflict was discussed in the original issue too.

@ilyavolodin
Copy link
Member

In that case LGTM.

@ilyavolodin ilyavolodin merged commit 0e14016 into master Jun 9, 2016
@mysticatea mysticatea deleted the no-mixed-operators/new branch June 9, 2016 16:56
@jfmengels
Copy link
Contributor

Woohoo! Thanks a lot folks! Especially @mysticatea :)

@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
No reviews
Assignees
No one assigned
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@mysticatea @eslintbot @mention-bot @jfmengels @michaelficarra @ilyavolodin @jquerybot