Skip to content
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

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

Closed
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
@MylesBorins
Copy link
Contributor

is this semver-minor? original was semver-major

@MylesBorins
Copy link
Contributor

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

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

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>
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>
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>
If the current working directory is removed, Node cannot
start normally because the module system calls uv_cwd().

Refs: nodejs#12022
PR-URL: nodejs#12343
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
Copy link
Member Author

Trott commented Sep 29, 2017

Rebase: Sure, done.

#12343: Cherry-picked over 4fc1199 and so that's in here now too. Take it out if you want to cherry-pick/land it separate from this PR.

MylesBorins pushed a commit that referenced this pull request Oct 10, 2017
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'

Backport-PR-URL: #15479
PR-URL: #12293
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 10, 2017
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.

Backport-PR-URL: #15479
PR-URL: #12293
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 10, 2017
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.

Backport-PR-URL: #15479
PR-URL: #12293
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 10, 2017
If the current working directory is removed, Node cannot
start normally because the module system calls uv_cwd().

Backport-PR-URL: #15479
Refs: #12022
PR-URL: #12343
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Contributor

landed in 1213f38...0597d3f

MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
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'

Backport-PR-URL: #15479
PR-URL: #12293
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
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.

Backport-PR-URL: #15479
PR-URL: #12293
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
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.

Backport-PR-URL: #15479
PR-URL: #12293
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
If the current working directory is removed, Node cannot
start normally because the module system calls uv_cwd().

Backport-PR-URL: #15479
Refs: #12022
PR-URL: #12343
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott Trott deleted the backport-12293 branch January 13, 2022 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert subsystem. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants