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

Use exceptRange for Yoda rule #1

Closed
wants to merge 1 commit into from

Conversation

janpaul123
Copy link

Llewellyn Falco makes an excellent argument for why to write things like 0 < x.

Alternatively, since the default setting is "never", you could consider this a stylistic
rule, and argue that it should not be in this config at all. (Since only when setting it
to "always" is it preventing problems, but also those problems are already prevented
by the no-cond-assign rule.)

Llewellyn Falco makes an excellent argument for why to write things like `0 < x`.

Alternatively, since the default setting is "never", you could consider this a stylistic
rule, and argue that it should not be in this config at all. (Since only when setting it
to "always" is it preventing problems, but also those problems are already prevented
by the `no-cond-assign` rule.)
@RyanZim RyanZim self-requested a review July 10, 2018 19:01
@RyanZim RyanZim self-assigned this Jul 10, 2018
@RyanZim
Copy link
Owner

RyanZim commented Jul 10, 2018

Will have a look later today

@janpaul123
Copy link
Author

Cool, thanks! Actually, I just noticed that exceptRange is not enough, as that only works if you do something like 0 < x && x < 10, but not when just doing 0 < x. I'd be in favour of removing this rule entirely.

@RyanZim
Copy link
Owner

RyanZim commented Jul 10, 2018

I personally strongly disagree with Llewellyn Falco here; he thinks in terms of a number line, I think in terms of reading the code. To me, (x > 5 && x < 10) is the clear way to do it; it reads as "x is greater than five and less than 10." I read (5 < x && x < 10) as "5 is less than x and x is less than 10," which is very unclear, since first you're talking about the number 5, and in the second half, you're talking about x. However, that's my personal opinion, and that isn't relevant to this discussion.

I think we all agree "red" === foo is bad. There's zero reason to do it if you have no-cond-assign enabled (which this config does). Whether yoda conditions are bad with relational operators (<, >, <=, >=) is debatable and stylistic. I never noticed that this rule applies to relational operators (since I personally always write code in compliance with the rule); this is why I added it.

The onlyEquality option is a possible solution, but it still allows "red" !== foo, which is dumb IMO. It's too bad there isn't a way to disallow it for === & !== but allow them for relational operators. Not sure what's best here.

@janpaul123
Copy link
Author

Yeah, that all makes sense to me. I think it's best then to just disable the rule altogether, because even though "red" === foo is ugly, it doesn't cause any problems, and in the end that's what this repo is all about.

@RyanZim
Copy link
Owner

RyanZim commented Jul 11, 2018

True, true; I see your point, but we also disallow this:

if (foo) return something()
else return somethingElse()

^ That isn't strictly a problem, but it's dumb and verbose.

How about yoda with onlyEquality: true? It'll miss !==, but at least it will enforce ===.

@janpaul123
Copy link
Author

Very true, so I guess it's up to you if you want to include "dumb and verbose" in the scope of this plugin. 😉 If that's what you want, then onlyEquality: true seems reasonable!

@RyanZim RyanZim closed this in 0597335 Jul 18, 2018
@RyanZim
Copy link
Owner

RyanZim commented Jul 18, 2018

Loosened with onlyEquality: true in v1.1.0 🎉

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

Successfully merging this pull request may close these issues.

None yet

2 participants