(require|disallow)YodaConditions: add support for listing specific operators by Slayer95 · Pull Request #1031 · jscs-dev/node-jscs

Slayer95 · 2015-02-09T14:28:48Z

coveralls · 2015-02-09T14:30:32Z

Coverage increased (+0.0%) to 99.0% when pulling ee117d3 on Slayer95:yoda-conditions-array into 0feb327 on jscs-dev:master.

hzoo · 2015-02-09T14:37:45Z

lib/rules/disallow-yoda-conditions.js

there's an extra != here

Removed it.

hzoo · 2015-02-09T14:41:17Z

looks good!

coveralls · 2015-02-09T14:46:55Z

Coverage increased (+0.0%) to 99.0% when pulling 3633159 on Slayer95:yoda-conditions-array into 0feb327 on jscs-dev:master.

Fixes jscs-dev#1026 Closes jscs-dev#1031

Fixes jscs-dev#1026 Closes jscs-devgh-1031

coveralls · 2015-02-09T14:56:22Z

Coverage increased (+0.0%) to 99.0% when pulling 6425ebd on Slayer95:yoda-conditions-array into 0feb327 on jscs-dev:master.

qfox · 2015-02-10T22:42:07Z

I thought about it. Look like we lost a reason of denying yoda conditions.

Afaik it was a bad code style because there is a difference between a = 5 and a == 5. And mistype is not a syntax error. But it's impossible to mistype a < 5. Do we really need this fix or we should just rethink this rule?

Slayer95 · 2015-02-10T23:49:23Z

I agree that yoda conditions are only useful to guard against that typo (i.e. requireYodaConditions can be limited to it. However, that's more easily dealt with other sort of rules, like JSHint's "Expected a conditional expression and instead saw an assignment".

Regarding disallowYodaConditions, an argument based on aesthetics and backwards compatibility can be made to keep supporting the current behavior, but I wouldn't mind its getting restricted to ==.

Interestingly, ESLint provides the option to forbid Yoda Conditions except for ranges, which is my exact use case.

mrjoelkemp · 2015-03-17T07:34:11Z

test/rules/disallow-yoda-conditions.js

Why was this removed from the beforeEach?

mrjoelkemp · 2015-03-17T07:34:53Z

Is this PR mergeable (once rebased)?

cc @zxqfox @hzoo

qfox · 2015-03-17T07:39:38Z

@mrjoelkemp I think it's too complicated and can be simplified. I'm not sure that 5 <= value should be interpreted as yoda condition.

Fixes jscs-dev#1026 Closes jscs-devgh-1031

coveralls · 2015-03-25T02:34:31Z

Coverage increased (+0.0%) to 99.09% when pulling ef4b343 on Slayer95:yoda-conditions-array into 02bf7ac on jscs-dev:master.

markelog · 2015-04-07T17:34:03Z

Is that still WIP?

Slayer95 · 2015-04-07T17:48:14Z

Waiting for feedback / replies to @zxqfox with details regarding what we want as destination.

I guess I should have used something like "WFF" rather than "WIP" -if that's a thing.

markelog · 2015-04-30T11:03:09Z

ping @zxqfox

qfox · 2015-05-31T11:35:33Z

lib/rules/disallow-yoda-conditions.js

What if make ['==', '===', '!='] by default (true)?

Even just '===' and '=='.
Historical reasons.

I'm not sure about this. As to me they are all Yoda-style. And still I can't see enough reasons to break backwards compatibility.

It wouldn't actually break anything though. That is, no failing builds on update.

I'm not talking about the builds. I'm about the unexpected behaviour change. For instance, I know several projects which rely on the current behaviour.

Actually I'm fine if we left it as is.

qfox · 2015-05-31T11:44:35Z

Sorry for delay, somehow missed this PR. ;-(

Fixes jscs-dev#1026 Closes jscs-devgh-1031

coveralls · 2015-06-04T06:09:23Z

Coverage increased (+0.0%) to 99.17% when pulling 7034ae9 on Slayer95:yoda-conditions-array into f963cd4 on jscs-dev:master.

Slayer95 · 2015-06-04T06:10:25Z

Updated tests as requested.

qfox · 2015-06-04T12:09:18Z

@Slayer95 Thanks!

LGTM

mdevils · 2015-06-04T14:13:40Z

LGTM! Good job!

markelog · 2015-06-08T04:05:15Z

Just a remainder, if pr has two "LGTM" you can definitely merge it, @mdevils, @zxqfox could one of you do that?

mdevils · 2015-06-10T07:30:14Z

@markelog Yes, sure :)

mdevils · 2015-06-10T07:59:46Z

Merged into 2.0. Thank you!

Fixes #1026 Closes gh-1031

hzoo reviewed Feb 9, 2015
View reviewed changes

Slayer95 force-pushed the yoda-conditions-array branch from 3633159 to 3508bbf Compare February 9, 2015 14:48

Slayer95 added a commit to Slayer95/node-jscs that referenced this pull request Feb 9, 2015

requireYodaConditions: support an array of operators

3508bbf

Fixes jscs-dev#1026 Closes jscs-dev#1031

Slayer95 force-pushed the yoda-conditions-array branch from 3508bbf to 8c9c772 Compare February 9, 2015 14:49

Slayer95 added a commit to Slayer95/node-jscs that referenced this pull request Feb 9, 2015

requireYodaConditions: support an array of operators

8c9c772

Fixes jscs-dev#1026 Closes jscs-devgh-1031

Slayer95 force-pushed the yoda-conditions-array branch from 8c9c772 to 6425ebd Compare February 9, 2015 14:54

Slayer95 added a commit to Slayer95/node-jscs that referenced this pull request Feb 9, 2015

requireYodaConditions: support an array of operators

6425ebd

Fixes jscs-dev#1026 Closes jscs-devgh-1031

mrjoelkemp reviewed Mar 17, 2015
View reviewed changes

test/rules/disallow-yoda-conditions.js Outdated

Copy link
Copy Markdown

Member

mrjoelkemp Mar 17, 2015

Choose a reason for hiding this comment

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

Why was this removed from the beforeEach?

Slayer95 force-pushed the yoda-conditions-array branch from 6425ebd to ef4b343 Compare March 25, 2015 02:19

Slayer95 added a commit to Slayer95/node-jscs that referenced this pull request Mar 25, 2015

requireYodaConditions: support an array of operators

ef4b343

Fixes jscs-dev#1026 Closes jscs-devgh-1031

Slayer95 changed the title ~~(require|disallow)YodaConditions: add support for listing specific operators~~ [WIP] (require|disallow)YodaConditions: add support for listing specific operators Mar 25, 2015

hzoo mentioned this pull request Apr 7, 2015

[Feature Request] (Disallow|Require)YodaConditions for specific operators #1026

Closed

qfox reviewed May 31, 2015
View reviewed changes

Slayer95 force-pushed the yoda-conditions-array branch from ef4b343 to dbfd67b Compare June 4, 2015 05:45

Slayer95 added a commit to Slayer95/node-jscs that referenced this pull request Jun 4, 2015

requireYodaConditions: support an array of operators

dbfd67b

Fixes jscs-dev#1026 Closes jscs-devgh-1031

Slayer95 added 2 commits June 4, 2015 00:56

disallowYodaConditions: support an array of operators

be2fe90

requireYodaConditions: support an array of operators

7034ae9

Fixes jscs-dev#1026 Closes jscs-devgh-1031

Slayer95 force-pushed the yoda-conditions-array branch from dbfd67b to 7034ae9 Compare June 4, 2015 06:02

qfox added the reviewed once label Jun 4, 2015

Slayer95 changed the title ~~[WIP] (require|disallow)YodaConditions: add support for listing specific operators~~ (require|disallow)YodaConditions: add support for listing specific operators Jun 4, 2015

mdevils pushed a commit that referenced this pull request Jun 10, 2015

requireYodaConditions: support an array of operators

074fd02

Fixes #1026 Closes gh-1031

mdevils closed this Jun 10, 2015

qfox mentioned this pull request Jul 1, 2015

yodaConditions: allow literals on the left side of comparison operators (<, >, <=, >=) #1503

Closed

markelog pushed a commit that referenced this pull request Jul 8, 2015

requireYodaConditions: support an array of operators

4596053

Fixes #1026 Closes gh-1031

Conversation

Slayer95 commented Feb 9, 2015

Uh oh!

coveralls commented Feb 9, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hzoo commented Feb 9, 2015

Uh oh!

coveralls commented Feb 9, 2015

Uh oh!

coveralls commented Feb 9, 2015

Uh oh!

qfox commented Feb 10, 2015

Uh oh!

Slayer95 commented Feb 10, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrjoelkemp commented Mar 17, 2015

Uh oh!

qfox commented Mar 17, 2015

Uh oh!

coveralls commented Mar 25, 2015

Uh oh!

markelog commented Apr 7, 2015

Uh oh!

Slayer95 commented Apr 7, 2015

Uh oh!

markelog commented Apr 30, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qfox commented May 31, 2015

Uh oh!

coveralls commented Jun 4, 2015

Uh oh!

Slayer95 commented Jun 4, 2015

Uh oh!

qfox commented Jun 4, 2015

Uh oh!

mdevils commented Jun 4, 2015

Uh oh!

markelog commented Jun 8, 2015

Uh oh!

mdevils commented Jun 10, 2015

Uh oh!

mdevils commented Jun 10, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants