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

assert: remove unneeded condition #11314

Closed
wants to merge 1 commit into from
Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented Feb 11, 2017

In expectedException(), actual is guaranteed to be truthy. We only need to check expected for truthiness.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

assert

@nodejs-github-bot nodejs-github-bot added the assert Issues and PRs related to the assert subsystem. label Feb 11, 2017
@mscdex
Copy link
Contributor

mscdex commented Feb 11, 2017

lib/assert.js Outdated
@@ -294,7 +294,8 @@ assert.notStrictEqual = function notStrictEqual(actual, expected, message) {
};

function expectedException(actual, expected) {
if (!actual || !expected) {
// actual is guaranteed to be an Error object, but we need to check expected
Copy link
Member

Choose a reason for hiding this comment

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

Nit: full stop at the end of the sentence. I assume actual doesn't need to be capitalised as it's a variable.

lib/assert.js Outdated
@@ -294,7 +294,8 @@ assert.notStrictEqual = function notStrictEqual(actual, expected, message) {
};

function expectedException(actual, expected) {
if (!actual || !expected) {
// actual is guaranteed to be an Error object, but we need to check expected
Copy link
Member

@gibfahn gibfahn Feb 12, 2017

Choose a reason for hiding this comment

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

Nit: full stop at the end of the sentence. I assume actual doesn't need to be capitalised as it's a variable.

EDIT: Not sure why this has appeared twice...

@gibfahn
Copy link
Member

gibfahn commented Feb 12, 2017

Also I guess we should note that this depends on #11304 (assuming dead code elimination doesn't count as a bugfix).

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@Trott
Copy link
Member Author

Trott commented Feb 15, 2017

assert is now Stable instead of Locked so this can land

@Trott
Copy link
Member Author

Trott commented Feb 15, 2017

Trott added a commit to Trott/io.js that referenced this pull request Feb 15, 2017
PR-URL: nodejs#11314
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@Trott
Copy link
Member Author

Trott commented Feb 15, 2017

Landed in 3429991

@Trott Trott closed this Feb 15, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 16, 2017
PR-URL: nodejs#11314
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
italoacasas pushed a commit that referenced this pull request Feb 22, 2017
PR-URL: #11314
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@italoacasas italoacasas mentioned this pull request Feb 25, 2017
jasnell pushed a commit that referenced this pull request Mar 7, 2017
PR-URL: #11314
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
jasnell pushed a commit that referenced this pull request Mar 7, 2017
PR-URL: #11314
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
PR-URL: #11314
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
PR-URL: #11314
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
@Trott Trott deleted the refactor-assert branch January 13, 2022 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants