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.

Sign up for GitHub

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

Jump to bottom

assert: remove unneeded condition #11314

Closed
wants to merge 1 commit into from
Closed

assert: remove unneeded condition #11314

wants to merge 1 commit into from

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

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

gibfahn
gibfahn reviewed Feb 12, 2017
View reviewed changes
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.

gibfahn
gibfahn reviewed Feb 12, 2017
View reviewed changes
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 •
edited
Loading

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).

mhdawson
mhdawson approved these changes Feb 14, 2017
View reviewed changes
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

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

Trott added a commit to Trott/io.js that referenced this pull request Feb 15, 2017
@Trott
assert: remove unneeded condition
3429991 
PR-URL: nodejs#11314
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@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
@Trott @italoacasas
assert: remove unneeded condition
1a33851 
PR-URL: nodejs#11314
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
italoacasas pushed a commit that referenced this pull request Feb 22, 2017
@Trott @italoacasas
assert: remove unneeded condition
6a2f330 
PR-URL: #11314
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@italoacasas italoacasas mentioned this pull request Feb 25, 2017
Merged
jasnell pushed a commit that referenced this pull request Mar 7, 2017
@Trott @jasnell
assert: remove unneeded condition
488cb02 
PR-URL: #11314
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
jasnell pushed a commit that referenced this pull request Mar 7, 2017
@Trott @jasnell
assert: remove unneeded condition
f266573 
PR-URL: #11314
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
@Trott @MylesBorins
assert: remove unneeded condition
32b264c 
PR-URL: #11314
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
@Trott @MylesBorins
assert: remove unneeded condition
090037a 
PR-URL: #11314
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
Merged
@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
Reviewers

@gibfahn gibfahn gibfahn left review comments

@jasnell jasnell jasnell approved these changes

@thefourtheye thefourtheye thefourtheye approved these changes

@lpinca lpinca lpinca approved these changes

@targos targos targos approved these changes

@cjihrig cjihrig cjihrig approved these changes

@mhdawson mhdawson mhdawson approved these changes

Assignees
No one assigned
Labels
assert Issues and PRs related to the assert subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@Trott @mscdex @gibfahn @jasnell @thefourtheye @lpinca @targos @cjihrig @mhdawson @nodejs-github-bot