assert: validate required arguments by BridgeAR · Pull Request #26641 · nodejs/node (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation14 Commits2 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

BridgeAR

This validates most assert functions to verify that the required
arguments are indeed passed to the function.

I defensively marked this as semver-major while I guess it could also be
called a bug fix (if any assertion passed with one argument, that's almost
certainly an error).

So if people are fine with this being a patch instead, I am happy to remove the label.

// cc @nodejs/linting @nodejs/assert

Checklist

@BridgeAR BridgeAR added the semver-major

PRs that contain breaking changes and should be released in the next major version.

label

Mar 13, 2019

@nodejs-github-bot

@BridgeAR

This validates most assert functions to verify that the required arguments are indeed passed to the function.

lpinca

@BridgeAR

@BridgeAR BridgeAR added the author ready

PRs that have at least one approval, no pending requests for changes, and a CI started.

label

Mar 13, 2019

Trott

Choose a reason for hiding this comment

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

LGTM other than the very minor change that I think some tests with the functions getting zero arguments should be added. (Currently, assert.strictEqual() does not raise an AssertionError or any kind of error. This will change that and we should confirm that behavior doesn't change later without us knowing it.)

I can see it being patch and won't block that if that's what everyone else thinks, but I think semver-major is the right call here. It's not essential that this change land in any current releases, so might as well be cautious.

Once those tests are added, feel free to re-request a review and I'll change to "approve".

@BridgeAR

Trott

Choose a reason for hiding this comment

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

LGTM. Nit: Might as well add for a.equal() just to get all of 'em, no? But I'm fine either way. Thanks for doing this PR!

@Trott

/ping @nodejs/tsc This is (cautiously) semver-major, so needs at least one more TSC approval.

@BridgeAR

@Trott if I should check all, I'll have to add six other tests (there are eight functions and I added tests for two that test no arguments at all in addition to checking one argument).

@BridgeAR

sam-github

Choose a reason for hiding this comment

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

LGTM

Its semver-major, assert.strictEqual(fu()) checks return value is undefined right now, and will throw invalid usage after. I don't think this is a fringe enough usage to wave away, or that there is any particular hurry to introduce this improvement to the arg checking.

@BridgeAR

@BridgeAR

@nodejs/tsc PTAL. This requires one more LG due to cautiously marking this as semver-major.

@Trott

jasnell

mcollina

Choose a reason for hiding this comment

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

LGTM

BridgeAR added a commit to BridgeAR/node that referenced this pull request

Mar 21, 2019

@BridgeAR

This validates most assert functions to verify that the required arguments are indeed passed to the function.

PR-URL: nodejs#26641 Reviewed-By: Luigi Pinca luigipinca@gmail.com Reviewed-By: Rich Trott rtrott@gmail.com Reviewed-By: Sam Roberts vieuxtech@gmail.com Reviewed-By: James M Snell jasnell@gmail.com Reviewed-By: Matteo Collina matteo.collina@gmail.com

@BridgeAR

BethGriggs added a commit that referenced this pull request

Apr 22, 2019

@BethGriggs

Notable changes:

PR-URL: #26930

BethGriggs added a commit that referenced this pull request

Apr 23, 2019

@BethGriggs

Notable changes:

PR-URL: #26930

BethGriggs added a commit that referenced this pull request

Apr 23, 2019

@BethGriggs

Notable changes:

PR-URL: #26930

This was referenced

Apr 23, 2019

@BridgeAR BridgeAR deleted the validate-assert-arguments branch

January 20, 2020 11:27

Reviewers

@sam-github sam-github sam-github approved these changes

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

@Trott Trott Trott approved these changes

@lpinca lpinca lpinca approved these changes

@targos targos Awaiting requested review from targos

@addaleax addaleax Awaiting requested review from addaleax

Labels

assert

Issues and PRs related to the assert subsystem.

author ready

PRs that have at least one approval, no pending requests for changes, and a CI started.

semver-major

PRs that contain breaking changes and should be released in the next major version.

tools

Issues and PRs related to the tools directory.