-
Notifications
You must be signed in to change notification settings - Fork 29.9k
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 deprecated assert.fail()
functionality
#27526
Conversation
This removes the `assert.fail()` signature with more than one argument.
Very interested in CITGM results for this. As you point out, things will still fail, just with a different error message.... |
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.
This is already runtime deprecated with a warning and I don't think anyone has complained thus far - so while there isn't a huge motivation to remove this in terms of maintenance cost - if this failed CITGMs it would have likely already happened in the runtime deprecation duration.
(Gzemnid might not be a bad idea here too.) |
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.
Tentatively LGTM if CITGM and/or gzemnid queries don't turn up anything too alarming.
(Needs at least one more @nodejs/tsc approval.) |
I get a lot of hits checking gzemnid but there are also quite a few false positives. The main problem with these is that are used as It would be nice if anyone would help looking for false positives to reduce the list below.
|
After thinking about this again for a while I believe it's actually fine to keep this deprecation around for a significantly longer time. It does not hurt keeping the code and even though this will likely not be an issue for users there's also not much we gain by doing it now. If we wait e.g., two more years people might have refactored their code and updated more cases but this is mainly used as a "this code branch should never be reached" thing and thus they will also not be notified that this API is deprecated. |
This removes the
assert.fail()
signature with more than one argument.It is runtime deprecated since v10 and even if the old signature is continued to be used, it'll work similar to before: it will fail but it'll have a different error message.
This should land after #27525 to reduce conflicts backporting the other PR.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes