Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34.4k
[v6.x backport] doc,assert: document stackStartFunction in fail#14427
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
refack commented Jul 22, 2017 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
refack commented Jul 22, 2017
refack commented Jul 22, 2017
refack commented Jul 22, 2017
|
f9419c2 to 403c465Compareaaf4e13 to 31f572cComparesam-github commented Sep 19, 2017
@refack rebase onto v6.x-master and I'm confused by the PR title "document ...", but it implements, as well as documents? Adding yet another argument to |
sam-github commented Sep 19, 2017
Staring slightly harder, this seems to be a couple changes squashed into one commit: both a refactor of lib/assert.js, and the documentation of a pre-existing API? @refack need some clarification here, please. |
ec0149a to d60e9b7CompareBridgeAR commented Sep 19, 2017
@sam-github it is indeed a combination of multiple commits and if I read the change correct it also changes assert.fail a tiny bit. It also includes e.g. #12293 |
refack commented Sep 19, 2017
MylesBorins commented Sep 19, 2017
Decided not to move forward |
refack commented Oct 10, 2017
d60e9b7 to ded595fCompare* refactor the code 1. Rename private functions 2. Use destructuring 3. Remove obsolete comments * remove eslint rule PR-URL: nodejs#13862 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
ded595f to eeab2a3Comparerefack commented Oct 10, 2017 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
MylesBorins commented Oct 10, 2017
@refack the lts working group decided to not land this independent of how hard it was to backport. We gate which minor updates we land on the branch. Closing again, feel free to continue discussion though. |
refack commented Oct 10, 2017
As far as I remember (from listening to the discussion) the decision was based on this PR affecting the output of The only change is improving the default error for no args diff --git a/test/parallel/test-assert-fail.js b/test/parallel/test-assert-fail.js index a64cfdb3abb..ebd962be751 100644 assert.throws( () =>{assert.fail()}, - /^AssertionError: undefined undefined undefined$/+ AssertionError,+ 'Failed' );So I'd like this to be reconsidered. |
gibfahn commented Oct 10, 2017
So the part that we thought was a blocker we've actually already backported? Seems a bit silly not to backport this now then. +1 to backporting. |
Refs: #13862
Was asked to port to
v8.xbut I ported tov6.xby mistake, so here it is if you want it 🤷♂️/cc @nodejs/lts
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
assert