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

lib: return boolean result of assert.match method #35115

Closed

Conversation

juanarbol
Copy link
Member

The assert.match method by default will throws if there is no match,
this change can add the function of instead of throwing or not, returns
the boolean result of match.

Fixes: #33869

Note: I'm going to leave the documentation as a TODO :)

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

The `assert.match` method by default will throws if there is no match,
this change can add the function of instead of throwing or not, returns
the boolean result of match.

Fixes: nodejs#33869
@nodejs-github-bot nodejs-github-bot added the assert Issues and PRs related to the assert subsystem. label Sep 9, 2020
lib/assert.js Outdated Show resolved Hide resolved
@addaleax
Copy link
Member

addaleax commented Sep 9, 2020

I think this effectively just gives you the same result as regexp.test(string), as it is currently implemented, and in that case regexp.test(string) should definitely be preferred, I think.

I would suggest either closing #33869 as wontfix – I agree with the concern voiced by the author that this would be unexpected for an assertion method – or following @lundibundi’s suggestion there and making all the underlying assertion mechanisms available as separate methods, not just assert.match

@juanarbol
Copy link
Member Author

I think this effectively just gives you the same result as regexp.test(string), as it is currently implemented, and in that case regexp.test(string) should definitely be preferred, I think.

:O haha, this is true...

I would suggest either closing #33869 as wontfix – I agree with the concern voiced by the author that this would be unexpected for an assertion method – or following @lundibundi’s suggestion there and making all the underlying assertion mechanisms available as separate methods, not just assert.match

That really make sense to my.

@juanarbol
Copy link
Member Author

Closing this, this is almost like regexp.test(string).

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

assert.match should return match result
4 participants