-
Notifications
You must be signed in to change notification settings - Fork 29k
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: use a default message in assert #18319
Conversation
I really can't decide how I feel about this... I can't even decide how I feel about the original change. I mean, |
@apapirovski yes, |
doc/api/assert.md
Outdated
@@ -664,6 +663,7 @@ property set equal to the value of the `message` parameter. If the `message` | |||
parameter is `undefined`, a default error message is assigned. If the `message` | |||
parameter is an instance of an [`Error`][] then it will be thrown instead of the | |||
`AssertionError`. | |||
If no arguments are passed in at all `message` will be set to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incomplete sentence?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uh, somehow I did not finish it... solved.
In case no arguments are passed to `assert.ok` it should just use a default message. Otherwise `assert.ok` can not be used as a callback.
40a6192
to
9fc44f2
Compare
Rebased due to conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Umm, press the button too soon...how does changing this to ERR_ASSERTION makes it possible to be used in callbacks?
@joyeecheung i tried to highlight that in the descriptionof the PR. It it all about not providing a value. An error will be thrown one way or the other, the question is what error. I personally reconsidered and think a default is best in such a case. |
@BridgeAR By default do you mean |
@joyeecheung yes, that is what I mean. It might be "expected" behavior to not pass in a value in that case. That would probably mainly be the case by using e.g. spread ( In both cases we notify the user that no arguments were passed to the function but with this PR it is "acceptable" to do so. With the current implementation it is considered a faulty behavior. |
@BridgeAR I think I understand now, thanks. With the example in OP:
The first one does seem to be less confusing, although I am not sure if |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@joyeecheung I changed a testcase to outline the use case further. New CI: https://ci.nodejs.org/job/node-test-pull-request/12806/ |
Landed in 3cd7977 |
In case no arguments are passed to `assert.ok` it should just use a default message. Otherwise `assert.ok` can not be used as a callback. PR-URL: nodejs#18319 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Weijia Wang <starkwang@126.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
In case no arguments are passed to `assert.ok` it should just use a default message. Otherwise `assert.ok` can not be used as a callback. PR-URL: nodejs#18319 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Weijia Wang <starkwang@126.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
In case no arguments are passed to
assert.ok
it should justuse a default message. Otherwise
assert.ok
can not be used asa callback.
The PR that changed the behavior before is
semver-major
and has not yet been released.So I just changed the original changed entry.
The reason for the change is something like this:
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
assert