assert: replace many if's with if-else statement #14043

viktor-ku · 2017-07-02T21:05:26Z

If assert.fail has less than 3 arguments then some conditions will
be applied. However after applying one of them there is no need to
check for others.

Tests are passing

Checklist

make -j4 test (UNIX)
commit message follows commit guidelines

Affected core subsystem(s)

assert

refack · 2017-07-02T21:31:35Z

Won't this conflict with #13973?

BridgeAR · 2017-07-02T21:37:57Z

Yes, it will

viktor-ku · 2017-07-02T21:40:12Z

Just in one place where lazyErrors where used. Should I remove that line?

refack · 2017-07-02T21:41:11Z

Just in one place where lazyErrors where used. I can just remove this line?

All in good time... Let's see how thing progress...

viktor-ku · 2017-07-02T21:42:23Z

Should I do something with this PR for it to progress? Or it is not depend on me?

refack · 2017-07-02T21:53:03Z

Should I do something with this PR for it to progress? Or it is not depend on me?

@kuroljov first let me say, welcome, and thank you for your contribution 🥇
If you are not familiar with our review and landing process, it's covered in CONTRIBUTING.md

For now this PR looks good, if #13973 get's blocked this might land first, if #13973 lands first you'll need to rebase...

viktor-ku · 2017-07-02T21:56:25Z

@refack Thank you. Well, let's see then

viktor-ku · 2017-07-03T09:53:12Z

I've resolved the conflicts

tniessen · 2017-07-03T14:12:31Z

@kuroljov It seems like you accidentally pushed a merge commit instead of your rebased changes. This makes it difficult to run CI against your changes and also requires a manual rebase while landing this PR. I assume you added https://github.com/nodejs/node.git as the upstream repository. If you did not, you can add it like this:

git remote add upstream https://github.com/nodejs/node.git

Please follow these steps to perform a proper rebase:

git checkout assert
git reset --hard 5362e0990ec01a1aad78123e05f4ceefb62f726f
git fetch upstream
git rebase upstream/master

There will likely be some conflicts at this point. Running git status shows you a list of "Unmerged paths", the files with conflicts will be marked as "both modified". After fixing the rebase conflicts in these files, you can continue:

git rebase --continue

If everything worked, you should have a single commit on top of our upstream master containing your changes. You can push the changes to your repository using

git push origin assert --force

If you need further assistance, please let me know.

(Based on #13852 (comment))

If assert.fail has less than 3 arguments then some conditions will be applied. However after applying one of them there is no need to check for others.

viktor-ku · 2017-07-03T18:27:08Z

@tniessen my bad. I pushed rebased assert branch and now it looks like it has no conflicts

refack · 2017-07-03T18:28:19Z

lib/assert.js

@@ -62,18 +62,20 @@ function innerFail(actual, expected, message, operator, stackStartFunction) {
 }

 function fail(actual, expected, message, operator, stackStartFunction) {
-  if (arguments.length === 0) {
+  const args = arguments.length;


Nit: Could you name it argsLen

I knew the argsLen will be the perfect name. Sure. Just give me a minute

refack

💯

tniessen · 2017-07-03T19:21:54Z

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

Replace multiple mutually exclusive `if` statements with if-else statements. PR-URL: #14043 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: David Cai <davidcai1993@yahoo.com>

tniessen · 2017-07-05T10:01:31Z

Landed in 6e86a70, thank you for your first contribution! 🎉 Keep up the good work! 🏅

CI on master: https://ci.nodejs.org/job/node-test-commit-linuxone/7086/

viktor-ku · 2017-07-05T11:59:01Z

@tniessen thank you

addaleax · 2017-07-22T20:21:38Z

This doesn’t seem to apply to v8.x, let me know if I’m mistaken.

viktor-ku · 2017-07-22T20:48:24Z

@addaleax not it was not

viktor-ku · 2017-09-14T07:56:37Z

@addaleax I misunderstood you. It can be landed on v8. Or if it can't then it definitely might be landed on v9. Sorry

nodejs-github-bot added the assert Issues and PRs related to the assert subsystem. label Jul 2, 2017

BridgeAR approved these changes Jul 2, 2017

View reviewed changes

lpinca approved these changes Jul 3, 2017

View reviewed changes

refack mentioned this pull request Jul 3, 2017

Dgram buffers #13623

Closed

4 tasks

assert: replace many if's with if-else statement

701e68e

If assert.fail has less than 3 arguments then some conditions will be applied. However after applying one of them there is no need to check for others.

refack approved these changes Jul 3, 2017

View reviewed changes

assert: rename args to argsLen

73c630c

refack approved these changes Jul 3, 2017

View reviewed changes

tniessen self-assigned this Jul 3, 2017

tniessen approved these changes Jul 3, 2017

View reviewed changes

cjihrig approved these changes Jul 3, 2017

View reviewed changes

fhinkel approved these changes Jul 3, 2017

View reviewed changes

DavidCai1111 approved these changes Jul 5, 2017

View reviewed changes

tniessen closed this Jul 5, 2017

viktor-ku deleted the assert branch July 6, 2017 03:06

addaleax added the dont-land-on-v4.x label Jul 22, 2017

addaleax added dont-land-on-v6.x labels Jul 22, 2017

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

assert: replace many if's with if-else statement #14043

assert: replace many if's with if-else statement #14043

viktor-ku commented Jul 2, 2017

refack commented Jul 2, 2017

BridgeAR commented Jul 2, 2017

viktor-ku commented Jul 2, 2017 •

edited

Loading

refack commented Jul 2, 2017

viktor-ku commented Jul 2, 2017

refack commented Jul 2, 2017 •

edited

Loading

viktor-ku commented Jul 2, 2017

viktor-ku commented Jul 3, 2017 •

edited

Loading

tniessen commented Jul 3, 2017

viktor-ku commented Jul 3, 2017

refack Jul 3, 2017

viktor-ku Jul 3, 2017

refack left a comment

tniessen commented Jul 3, 2017

tniessen commented Jul 5, 2017

viktor-ku commented Jul 5, 2017

addaleax commented Jul 22, 2017

viktor-ku commented Jul 22, 2017

viktor-ku commented Sep 14, 2017

assert: replace many if's with if-else statement #14043

assert: replace many if's with if-else statement #14043

Conversation

viktor-ku commented Jul 2, 2017

Checklist

Affected core subsystem(s)

refack commented Jul 2, 2017

BridgeAR commented Jul 2, 2017

viktor-ku commented Jul 2, 2017 • edited Loading

refack commented Jul 2, 2017

viktor-ku commented Jul 2, 2017

refack commented Jul 2, 2017 • edited Loading

viktor-ku commented Jul 2, 2017

viktor-ku commented Jul 3, 2017 • edited Loading

tniessen commented Jul 3, 2017

viktor-ku commented Jul 3, 2017

refack Jul 3, 2017

Choose a reason for hiding this comment

viktor-ku Jul 3, 2017

Choose a reason for hiding this comment

refack left a comment

Choose a reason for hiding this comment

tniessen commented Jul 3, 2017

tniessen commented Jul 5, 2017

viktor-ku commented Jul 5, 2017

addaleax commented Jul 22, 2017

viktor-ku commented Jul 22, 2017

viktor-ku commented Sep 14, 2017

viktor-ku commented Jul 2, 2017 •

edited

Loading

refack commented Jul 2, 2017 •

edited

Loading

viktor-ku commented Jul 3, 2017 •

edited

Loading