lib: remove some yoda statements#18746
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added the lib / src 
TimothyGu left a comment
TimothyGu left a comment There was a problem hiding this comment.
Commit message nit: s/yoda/Yoda conditions/. Otherwise LGTM.
There was a problem hiding this comment.
The !== null test should be kept to make the code more explicit.
There was a problem hiding this comment.
A couple of our current tests rely on the implicit version. Do you want those also be converted? Because I would argue having a consistent style is best.
There was a problem hiding this comment.
Yes, but I'd rather have that in a separate PR.
doc/api/stream.md Outdated
There was a problem hiding this comment.
Type-check-on-RHS is less readable than the other way around in expressions like these, IMO.
There was a problem hiding this comment.
I think this is something very subjective. I personally feel like the version is better to read.
lib/_tls_wrap.js Outdated
There was a problem hiding this comment.
Another example where I think it hurts legibility: the line is so long that the type check almost falls off the screen.
There was a problem hiding this comment.
Here the process.env part is actually something I consider less important to look at. So I have to move further to see that the comparison is with the NODE_TLS_REJECT_UNAUTHORIZED env.
There was a problem hiding this comment.
Dropping the strict comparison is a semantic change that I'm not sure is actually correct. Same comment applies to a couple of other places.
There was a problem hiding this comment.
Write operations should always either return true or false. If that is not the case, it would be an error.
For read operations it returns a string, a buffer or null. The latter when there is nothing to read. Buffer is always truthy and the string should (to my best knowledge) always be non empty. I might be wrong about the empty string case though. But here in our tests we definitely do not have any empty strings.
There was a problem hiding this comment.
Write operations should always either return true or false.
In the past we treated (for example) undefined different from false. In fact, I suspect the change to lib/internal/streams/legacy.js is a subtle regression that just happens to fly under the radar of our test suite.
jasnell commented Feb 13, 2018
I'm mildly -0 on this. Personally not a fan of Yoda-style, no, but live with it, I can. Agree with @bnoordhuis, I do, that more readable sometimes it is. |
BridgeAR commented Feb 13, 2018
Alright, shall I remove the rule and keep most changes or should I just close the PR? |
BridgeARforce-pushed the remove-yoda branch 3 times, most recently from b036efd to 7bf1793CompareBridgeAR commented Feb 16, 2018
I updated the PR to only remove a couple yoda statements. PTAL. I am also OK with closing this if that is the preference. |
BridgeAR commented Feb 17, 2018
Rebased due to conflicts. |
BridgeAR commented Feb 21, 2018
How shall we progress here? @jasnell@bnoordhuis |
bnoordhuis commented Feb 21, 2018
When in doubt, do nothing. I'm not 100% sure but I think it's a backwards-incompatible change in behavior. |
gibfahn commented Feb 21, 2018
Or you could just leave the Reversing the yoda expressions seems fine to me. |
jasnell commented Feb 22, 2018
I'm -0 on the entire change. Not favorable but won't block and defer to @bnoordhuis' opinion on it. |
BridgeAR commented Mar 2, 2018
I further reduced the changeset and think the rest should probably be uncontroversial. PTAL @bnoordhuis@jasnell@gibfahn@TimothyGu |
BridgeAR commented Mar 2, 2018
Mini-CI just for the reduced case: https://ci.nodejs.org/job/node-test-commit-light/329/ |
| code===0x2329||// LEFT-POINTING ANGLE BRACKET | ||
| code===0x232a||// RIGHT-POINTING ANGLE BRACKET | ||
| // CJK Radicals Supplement .. Enclosed CJK Letters and Months | ||
| (0x2e80<=code&&code<=0x3247&&code!==0x303f)|| |
There was a problem hiding this comment.
Most of the changes seem good to me but I wonder if this whole block is the one exception? It's basically making it look like 0 <= code <= 100, which is actually kinda nice readability wise.
There was a problem hiding this comment.
Do you feel strongly about it? Otherwise I would just go ahead and land it as is.
apapirovski commented Mar 4, 2018
Landed in f2d9379 |
PR-URL: #18746 Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#18746 Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#18746 Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>
jasnell commented Aug 17, 2018
Should this be backported to 8.x? If so, a separate backport PR is needed. |
7 participants
This enables the yoda eslint rule and prohibits statements likefalse === variable. They are mainly historically and if someone would add a new one, I guess it would be complained about while reviewing. This makes that complain obsolete due to the lint rule.I fixed the cases where this failed.
Note: this is currently blocked by #18395 as that removes a lot of yoda statements as well. So I only removed the other cases here.Update: I kept some yoda statements and removed the rule.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
lib, benchmark, doc, test