lib: remove some yoda statements #18746

BridgeAR · 2018-02-13T02:34:55Z

This enables the yoda eslint rule and prohibits statements like false === 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), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

lib, benchmark, doc, test

TimothyGu

Commit message nit: s/yoda/Yoda conditions/. Otherwise LGTM.

TimothyGu · 2018-02-13T02:48:13Z

test/parallel/test-stream-push-strings.js

@@ -55,7 +55,7 @@ const ms = new MyStream();
 const results = [];
 ms.on('readable', function() {
  let chunk;
-  while (null !== (chunk = ms.read()))
+  while (chunk = ms.read())


The !== null test should be kept to make the code more explicit.

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.

Yes, but I'd rather have that in a separate PR.

bnoordhuis

Boo! Yoda style I like.

bnoordhuis · 2018-02-13T09:59:57Z

doc/api/stream.md

@@ -937,7 +937,7 @@ the internal buffer is fully drained.
 const readable = getReadableStreamSomehow();
 readable.on('readable', () => {
  let chunk;
-  while (null !== (chunk = readable.read())) {
+  while ((chunk = readable.read()) !== null) {


Type-check-on-RHS is less readable than the other way around in expressions like these, IMO.

I think this is something very subjective. I personally feel like the version is better to read.

bnoordhuis · 2018-02-13T10:01:42Z

lib/_tls_wrap.js

@@ -1099,7 +1099,7 @@ exports.connect = function(...args /* [port,] [host,] [options,] [cb] */) {
  var cb = args[1];

  var defaults = {
-    rejectUnauthorized: '0' !== process.env.NODE_TLS_REJECT_UNAUTHORIZED,
+    rejectUnauthorized: process.env.NODE_TLS_REJECT_UNAUTHORIZED !== '0',


Another example where I think it hurts legibility: the line is so long that the type check almost falls off the screen.

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.

bnoordhuis · 2018-02-13T10:03:39Z

test/parallel/test-http-pipeline-flood.js

@@ -78,6 +78,6 @@ function child() {
  conn.on('drain', common.mustCall(write));

  function write() {
-    while (false !== conn.write(req, 'ascii'));
+    while (conn.write(req, 'ascii'));


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.

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.

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 · 2018-02-13T23:33:53Z

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 · 2018-02-13T23:41:17Z

Alright, shall I remove the rule and keep most changes or should I just close the PR?

BridgeAR · 2018-02-16T19:34:55Z

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 · 2018-02-17T16:39:41Z

Rebased due to conflicts.

CI https://ci.nodejs.org/job/node-test-pull-request/13243/

BridgeAR · 2018-02-21T16:57:55Z

How shall we progress here? @jasnell @bnoordhuis

bnoordhuis · 2018-02-21T17:32:54Z

When in doubt, do nothing. I'm not 100% sure but I think it's a backwards-incompatible change in behavior.

gibfahn · 2018-02-21T17:41:03Z

When in doubt, do nothing. I'm not 100% sure but I think it's a backwards-incompatible change in behavior.

Or you could just leave the === false in and have this PR just reverse the yoda expressions. Removing === false should really be a separate PR anyway IMO. FWIW I much prefer being explicit about comparisons.

Reversing the yoda expressions seems fine to me.

jasnell · 2018-02-22T07:27:59Z

I'm -0 on the entire change. Not favorable but won't block and defer to @bnoordhuis' opinion on it.

BridgeAR · 2018-03-02T02:51:25Z

I further reduced the changeset and think the rest should probably be uncontroversial. PTAL @bnoordhuis @jasnell @gibfahn @TimothyGu

BridgeAR · 2018-03-02T02:52:28Z

Mini-CI just for the reduced case: https://ci.nodejs.org/job/node-test-commit-light/329/

apapirovski · 2018-03-02T18:19:07Z

lib/internal/readline.js

        // CJK Radicals Supplement .. Enclosed CJK Letters and Months
-        (0x2e80 <= code && code <= 0x3247 && code !== 0x303f) ||


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.

Do you feel strongly about it? Otherwise I would just go ahead and land it as is.

apapirovski · 2018-03-04T14:33:52Z

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]>

jasnell · 2018-08-17T22:42:26Z

Should this be backported to 8.x? If so, a separate backport PR is needed.

BridgeAR added the blocked PRs that are blocked by other issues or PRs. label Feb 13, 2018

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Feb 13, 2018

TimothyGu approved these changes Feb 13, 2018

View reviewed changes

bnoordhuis reviewed Feb 13, 2018

View reviewed changes

BridgeAR force-pushed the remove-yoda branch 3 times, most recently from b036efd to 7bf1793 Compare February 16, 2018 19:32

BridgeAR removed the blocked PRs that are blocked by other issues or PRs. label Feb 16, 2018

BridgeAR force-pushed the remove-yoda branch from 7bf1793 to f4e55c0 Compare February 17, 2018 16:39

BridgeAR changed the title ~~lib: prohibit yoda~~ lib: remove some yoda statements Feb 17, 2018

lib,test: remove yoda statements

2bfed5b

BridgeAR force-pushed the remove-yoda branch from f4e55c0 to 2bfed5b Compare March 2, 2018 02:50

bnoordhuis approved these changes Mar 2, 2018

View reviewed changes

jasnell approved these changes Mar 2, 2018

View reviewed changes

apapirovski reviewed Mar 2, 2018

View reviewed changes

apapirovski closed this Mar 4, 2018

apapirovski pushed a commit that referenced this pull request Mar 4, 2018

lib,test: remove yoda statements

f2d9379

PR-URL: #18746 Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

MylesBorins mentioned this pull request Mar 6, 2018

v9.8.0 proposal #19181

Merged

jasnell added the backport-requested-v8.x label Aug 17, 2018

BridgeAR deleted the remove-yoda branch April 1, 2019 23:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: remove some yoda statements #18746

lib: remove some yoda statements #18746

BridgeAR commented Feb 13, 2018 •

edited

Loading

TimothyGu left a comment

TimothyGu Feb 13, 2018

BridgeAR Feb 13, 2018

TimothyGu Feb 14, 2018

bnoordhuis left a comment

bnoordhuis Feb 13, 2018

BridgeAR Feb 13, 2018

bnoordhuis Feb 13, 2018

BridgeAR Feb 13, 2018

bnoordhuis Feb 13, 2018

BridgeAR Feb 13, 2018

bnoordhuis Feb 17, 2018

jasnell commented Feb 13, 2018

BridgeAR commented Feb 13, 2018

BridgeAR commented Feb 16, 2018

BridgeAR commented Feb 17, 2018

BridgeAR commented Feb 21, 2018

bnoordhuis commented Feb 21, 2018

gibfahn commented Feb 21, 2018

jasnell commented Feb 22, 2018

BridgeAR commented Mar 2, 2018

BridgeAR commented Mar 2, 2018

apapirovski Mar 2, 2018

BridgeAR Mar 2, 2018

apapirovski Mar 4, 2018

apapirovski commented Mar 4, 2018

jasnell commented Aug 17, 2018

		// CJK Radicals Supplement .. Enclosed CJK Letters and Months
		(0x2e80 <= code && code <= 0x3247 && code !== 0x303f) \|\|

lib: remove some yoda statements #18746

lib: remove some yoda statements #18746

Conversation

BridgeAR commented Feb 13, 2018 • edited Loading

Checklist

Affected core subsystem(s)

TimothyGu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Feb 13, 2018

BridgeAR commented Feb 13, 2018

BridgeAR commented Feb 16, 2018

BridgeAR commented Feb 17, 2018

BridgeAR commented Feb 21, 2018

bnoordhuis commented Feb 21, 2018

gibfahn commented Feb 21, 2018

jasnell commented Feb 22, 2018

BridgeAR commented Mar 2, 2018

BridgeAR commented Mar 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski commented Mar 4, 2018

jasnell commented Aug 17, 2018

BridgeAR commented Feb 13, 2018 •

edited

Loading