assert: use object argument in innerFail by BridgeAR · Pull Request #17582 · nodejs/node · GitHub
Skip to content

assert: use object argument in innerFail#17582

Closed
BridgeAR wants to merge 1 commit into
nodejs:masterfrom
BridgeAR:improve-innerFail
Closed

assert: use object argument in innerFail#17582
BridgeAR wants to merge 1 commit into
nodejs:masterfrom
BridgeAR:improve-innerFail

Conversation

@BridgeAR

@BridgeAR BridgeAR commented Dec 9, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

Right now it is difficult to know what argument stands for what
property. By refactoring the arguments into a object it is clear
what it means (It happened before that they got mixed up. See #17575).

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

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)

assert

@nodejs-github-bot nodejs-github-bot added assert Issues and PRs related to the assert subsystem. errors Issues and PRs related to JavaScript errors originated in Node.js core. labels Dec 9, 2017
@addaleax

addaleax commented Dec 9, 2017

Copy link
Copy Markdown
Member

@BridgeAR

BridgeAR commented Dec 9, 2017

Copy link
Copy Markdown
Member Author

@addaleax jupp, I only ran the assert tests. I now added a specific test for that to catch something not in a completely independent one. Thanks for pointing it out.

@BridgeAR

BridgeAR commented Dec 12, 2017

Copy link
Copy Markdown
Member Author

CI https://ci.nodejs.org/job/node-test-commit-light/19/

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 12, 2017
@apapirovski

apapirovski commented Dec 14, 2017

Copy link
Copy Markdown
Contributor

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

Some of the Windows tests didn't run last time. Just being extra safe.

@BridgeAR
assert: use object argument in innerFail
1cd7c4d 
Right now it is difficult to know what argument stands for what
property. By refactoring the arguments into a object it is clear
what stands for what.
@BridgeAR

BridgeAR commented Dec 15, 2017

Copy link
Copy Markdown
Member Author

New CI due to a rebase https://ci.nodejs.org/job/node-test-commit-light/28/

@BridgeAR

BridgeAR commented Dec 15, 2017

Copy link
Copy Markdown
Member Author

Landed in 7cf569a

@BridgeAR BridgeAR closed this Dec 15, 2017
BridgeAR added a commit to BridgeAR/node that referenced this pull request Dec 15, 2017
@BridgeAR
assert: use object argument in innerFail
7cf569a 
Right now it is difficult to know what argument stands for what
property. By refactoring the arguments into a object it is clear
what stands for what.

PR-URL: nodejs#17582
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Dec 28, 2017
@BridgeAR
assert: fix strict regression
64571fa 
Using `assert()` or `assert.ok()` resulted in a error since a
refactoring.

Refs: nodejs#17582
@BridgeAR BridgeAR mentioned this pull request Dec 28, 2017
Closed
4 tasks
BridgeAR added a commit to BridgeAR/node that referenced this pull request Dec 28, 2017
@BridgeAR
assert: fix strict regression
8578fe2 
Using `assert()` or `assert.ok()` resulted in a error since a
refactoring.

Refs: nodejs#17582

PR-URL: nodejs#17903
Refs: nodejs#17582
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 29, 2017
@MylesBorins

MylesBorins commented Jan 9, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

This does not land cleanly on v9.x, could it please be backported

edit: it should come with #17903

BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 8, 2018
@BridgeAR
assert: use object argument in innerFail
da78022 
Right now it is difficult to know what argument stands for what
property. By refactoring the arguments into a object it is clear
what stands for what.

PR-URL: nodejs#17582
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 8, 2018
@BridgeAR
assert: fix strict regression
fe9f098 
Using `assert()` or `assert.ok()` resulted in a error since a
refactoring.

Refs: nodejs#17582

PR-URL: nodejs#17903
Refs: nodejs#17582
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Mar 8, 2018
Closed
4 tasks
@BridgeAR

BridgeAR commented Mar 8, 2018

Copy link
Copy Markdown
Member Author

Backport opened in #19230

MylesBorins pushed a commit that referenced this pull request Mar 15, 2018
@BridgeAR @MylesBorins
assert: use object argument in innerFail
a959390 
Right now it is difficult to know what argument stands for what
property. By refactoring the arguments into a object it is clear
what stands for what.

Backport-PR-URL: #19230
PR-URL: #17582
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins pushed a commit that referenced this pull request Mar 15, 2018
@BridgeAR @MylesBorins
assert: fix strict regression
bf036a6 
Using `assert()` or `assert.ok()` resulted in a error since a
refactoring.

Refs: #17582

Backport-PR-URL: #19230
PR-URL: #17903
Refs: #17582
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos mentioned this pull request Mar 18, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
@BridgeAR @MylesBorins
assert: use object argument in innerFail
db73d1c 
Right now it is difficult to know what argument stands for what
property. By refactoring the arguments into a object it is clear
what stands for what.

Backport-PR-URL: #19230
PR-URL: #17582
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
@BridgeAR @MylesBorins
assert: fix strict regression
f96ea47 
Using `assert()` or `assert.ok()` resulted in a error since a
refactoring.

Refs: #17582

Backport-PR-URL: #19230
PR-URL: #17903
Refs: #17582
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins

MylesBorins commented Jul 31, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

Should this be backported to v8.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

Requested backport to 8.x in #19230

@LinusU

LinusU commented Aug 8, 2018

Copy link
Copy Markdown
Contributor

BridgeAR added a commit to BridgeAR/node that referenced this pull request Oct 2, 2018
@BridgeAR
assert: use object argument in innerFail
c947fe7 
Right now it is difficult to know what argument stands for what
property. By refactoring the arguments into a object it is clear
what stands for what.

PR-URL: nodejs#17582
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Oct 2, 2018
@BridgeAR
assert: fix strict regression
025d7de 
Using `assert()` or `assert.ok()` resulted in a error since a
refactoring.

Refs: nodejs#17582

PR-URL: nodejs#17903
Refs: nodejs#17582
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 31, 2018
@BridgeAR @MylesBorins
assert: use object argument in innerFail
a42d072 
Right now it is difficult to know what argument stands for what
property. By refactoring the arguments into a object it is clear
what stands for what.

Backport-PR-URL: #23223
PR-URL: #17582
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins pushed a commit that referenced this pull request Oct 31, 2018
@BridgeAR @MylesBorins
assert: fix strict regression
562787e 
Using `assert()` or `assert.ok()` resulted in a error since a
refactoring.

Refs: #17582

Backport-PR-URL: #23223
PR-URL: #17903
Refs: #17582
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Nov 12, 2018
Merged
@beevelop beevelop mentioned this pull request Nov 20, 2018
Closed
@BridgeAR BridgeAR deleted the improve-innerFail branch April 1, 2019 23:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes
@addaleax addaleax addaleax approved these changes
+2 more reviewers
@apapirovski apapirovski apapirovski approved these changes
@maclover7 maclover7 maclover7 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

assert Issues and PRs related to the assert subsystem. errors Issues and PRs related to JavaScript errors originated in Node.js core.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@BridgeAR @addaleax @apapirovski @MylesBorins @LinusU @jasnell @maclover7 @targos @nodejs-github-bot