8000 [v6.x backport] assert.fail() accept a single argument or two arguments by Trott · Pull Request #15479 · nodejs/node · GitHub
[go: up one dir, main page]
Skip to content

[v6.x backport] assert.fail() accept a single argument or two arguments #15479

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
Trott wants to merge 4 commits into from
Closed

[v6.x backport] assert.fail() accept a single argument or two arguments #15479

Trott wants to merge 4 commits into from

Conversation

@Trott
Copy link
Member
@Trott Trott commented Sep 19, 2017

Backport of #12293

First commit:

assert.fail() has two possible function signatures, both of which are
not intuitive. It virtually guarantees that people who try to use
assert.fail() without carefully reading the docs will end up using it
incorrectly.

This change maintains backwards compatibility with the two valid uses
(arguments 1 2 and 4 supplied but argument 3 falsy, and argument 3
supplied but arguments 1 2 and 4 all falsy) but also adds the far more
intuitive first-argument-only and first-two-arguments-only
possibilities.

assert.fail('boom');
// AssertionError: boom

assert.fail('a', 'b');
// AssertionError: 'a' != 'b'

Second commit: Remove lint rule that flags use of assert.fail() with a single argument.

Third commit: Remove common.fail() in tests since assert.fail() with a single argument works as expected.

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)

@nodejs-github-bot nodejs-github-bot added assert Issues and PRs related to the assert subsystem. tools Issues and PRs related to the tools directory. v6.x labels Sep 19, 2017
@Trott Trott mentioned this pull request Sep 19, 2017
Closed
4 tasks
@MylesBorins
Copy link
Contributor
MylesBorins commented Sep 20, 2017

is this semver-minor? original was semver-major

@MylesBorins
Copy link
Contributor
MylesBorins commented Sep 22, 2017

ping @Trott

@Trott
Copy link
Member Author
Trott commented Sep 22, 2017

@MylesBorins Original was labeled semver-major but I argued in #12293 (comment) that it was semver-minor. I argued in #12293 (comment) for backporting (and again that it wasn't really semver major). That comment got a 👍 from @addaleax @refack, and @ljharb. Subsequently, @sam-github and @BridgeAR endorsed backporting. @gibfahn also commented that it should go on v6.x., although it's not clear to me that they realized it was labeled semver-major.

I'm not seeing anyone arguing that it is semver major or that it shouldn't be backported with the possible exception of @refack.

@gibfahn
Copy link
Member
gibfahn commented Sep 23, 2017

To clarify, assuming you're talking about #12293 (comment), I was just saying that if this is backported to v6.x, then it should come with #12343.

Given #12293 (comment) though, I would say that IMO this isn't semver-major in practice. It's a notable change that we should call out, but I don't think it's going to break anyone.

@MylesBorins
Copy link
Contributor
MylesBorins commented Sep 29, 2017

@Trott would you be able to rebase and include #12343?

@Trott
assert: improve assert.fail() API
d25b8de 
assert.fail() has two possible function signatures, both of which are
not intuitive. It virtually guarantees that people who try to use
assert.fail() without carefully reading the docs will end up using it
incorrectly.

This change maintains backwards compatibility with the two valid uses
(arguments 1 2 and 4 supplied but argument 3 falsy, and argument 3
supplied but arguments 1 2 and 4 all falsy) but also adds the far more
intuitive first-argument-only and first-two-arguments-only
possibilities.

assert.fail('boom');
// AssertionError: boom

assert.fail('a', 'b');
// AssertionError: 'a' != 'b'

PR-URL: nodejs#12293
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
tools: remove assert.fail() lint rule
7c10194 
assert.fail() has been updated to accept a single argument so that is no
longer an error. Remove lint rule that checks for assert.fail() with a
single argument.

PR-URL: nodejs#12293
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
test: remove common.fail()
e5b11f8 
common.fail() was added to paste over issues with assert.fail() function
signature. assert.fail() has been updated to accept a single argument so
common.fail() is no longer necessary.

PR-URL: nodejs#12293
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@cjihrig @Trott
test: add cwd ENOENT known issue test