Skip to content

assert: deprecate assert.fail partially #18418

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
Closed
BridgeAR wants to merge 1 commit into from
Closed

assert: deprecate assert.fail partially #18418

BridgeAR wants to merge 1 commit into from

Conversation

@BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Jan 28, 2018

Using assert.fail() with more than one argument is not intuitive
to use and has no benefit over using a message on its own.

Therefore this introduces a runtime deprecation in case it is used
in that way.

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 the assert Issues and PRs related to the assert subsystem. label Jan 28, 2018
@BridgeAR BridgeAR force-pushed the deprecate-assert-fail branch from b990b8a to 33671e2 Compare January 28, 2018 12:05
@ChALkeR ChALkeR added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jan 28, 2018
lpinca
lpinca approved these changes Jan 28, 2018
View reviewed changes
doc/api/assert.md Outdated
Copy link
Member

@lpinca lpinca Jan 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: space after first _.

lib/assert.js Outdated
Copy link
Member

@lpinca lpinca Jan 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: assert.strictEqual() for consistency.

vsemozhetbyt
vsemozhetbyt reviewed Jan 28, 2018
View reviewed changes
doc/api/deprecations.md Outdated
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Jan 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: a individual -> an individual.

@BridgeAR
Copy link
Member Author

BridgeAR commented Jan 28, 2018

Comments addressed.

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

joyeecheung
joyeecheung reviewed Jan 29, 2018
View reviewed changes
doc/api/assert.md Outdated
Copy link
Member

@joyeecheung joyeecheung Jan 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The signature here looks rather confusing, since actual and expected looks required here...maybe create another section with assert.fail(message), and deprecate this signature while referencing the new one?

Copy link
Member

@joyeecheung joyeecheung Jan 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, that way we can put a deprecated entry in the YAML so the whole signature would be labeled with deprecated in the TOC.

Copy link
Member Author

@BridgeAR BridgeAR Jan 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it looks confusing. We just never had such a case before but I agree that this is probably the best way to solve this. I will change it shortly.

Copy link
Member Author

@BridgeAR BridgeAR Feb 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@BridgeAR
Copy link
Member Author

BridgeAR commented Jan 29, 2018

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

vsemozhetbyt
vsemozhetbyt reviewed Jan 29, 2018
View reviewed changes
doc/api/assert.md Outdated
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Jan 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary underscore?

jasnell
jasnell reviewed Jan 29, 2018
View reviewed changes
doc/api/assert.md Outdated
Copy link
Member

@jasnell jasnell Jan 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps drop the *:Note*: part of this now? It's not super necessary.

jasnell
jasnell reviewed Jan 29, 2018
View reviewed changes
doc/api/deprecations.md Outdated
Copy link
Member

@jasnell jasnell Jan 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/DEP0XXX/DEP00XX

joyeecheung
joyeecheung approved these changes Feb 1, 2018
View reviewed changes
Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with one suggestion

test/parallel/test-assert-fail.js Outdated
Copy link
Member

@joyeecheung joyeecheung Feb 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you put the tests with the valid signatures to another file?

Copy link
Member Author

@BridgeAR BridgeAR Feb 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@BridgeAR BridgeAR force-pushed the deprecate-assert-fail branch from 0c6d14a to 34143dd Compare February 2, 2018 15:19
@BridgeAR
Copy link
Member Author

BridgeAR commented Feb 2, 2018

Rebased due to conflicts and while doing so I also rewrote common.expectsError to assert.throws and found a mistake in one of the old tests because of that and fixed it. I also addressed @joyeecheung comment to split the tests into two files.

@BridgeAR
Copy link
Member Author

BridgeAR commented Feb 2, 2018

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

mcollina
mcollina approved these changes Feb 2, 2018
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but needs a rebase

@BridgeAR BridgeAR force-pushed the deprecate-assert-fail branch from 34143dd to adc742e Compare February 2, 2018 17:36
@BridgeAR
assert: deprecate assert.fail partially
bf4ac0c 
Using `assert.fail()` with more than one argument is not intuitive
to use and has no benefit over using a message on its own.

Therefore this introduces a runtime deprecation in case it is used
in that way.
@BridgeAR BridgeAR force-pushed the deprecate-assert-fail branch from adc742e to bf4ac0c Compare February 2, 2018 17:41
@BridgeAR
Copy link
Member Author

BridgeAR commented Feb 2, 2018

Landed in 70dcacd

@BridgeAR BridgeAR closed this Feb 2, 2018
BridgeAR added a commit to BridgeAR/node that referenced this pull request Feb 2, 2018
@BridgeAR
assert: deprecate assert.fail partially
70dcacd 
Using `assert.fail()` with more than one argument is not intuitive
to use and has no benefit over using a message on its own.

Therefore this introduces a runtime deprecation in case it is used
in that way.

PR-URL: nodejs#18418
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@BridgeAR @MayaLekova
assert: deprecate assert.fail partially
499de21 
Using `assert.fail()` with more than one argument is not intuitive
to use and has no benefit over using a message on its own.

Therefore this introduces a runtime deprecation in case it is used
in that way.

PR-URL: nodejs#18418
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@tniessen tniessen added the deprecations Issues and PRs related to deprecations. label Sep 7, 2018
@BridgeAR BridgeAR deleted the deprecate-assert-fail branch April 1, 2019 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

@lpinca lpinca lpinca approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

+2 more reviewers

@vsemozhetbyt vsemozhetbyt vsemozhetbyt left review comments

@apapirovski apapirovski apapirovski 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. deprecations Issues and PRs related to deprecations. semver-major PRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@BridgeAR @apapirovski @mcollina @jasnell @lpinca @joyeecheung @vsemozhetbyt @ChALkeR @tniessen @nodejs-github-bot