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.

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

assert: apply minor refactoring #11511

Closed
wants to merge 1 commit into from
Closed

assert: apply minor refactoring #11511

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 2 additions & 6 deletions lib/assert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,3 @@
// http://wiki.commonjs.org/wiki/Unit_Testing/1.0
//
// THIS IS NOT TESTED NOR LIKELY TO WORK OUTSIDE V8!
//
// Originally from narwhal.js (http://narwhaljs.org)
// Copyright (c) 2009 Thomas Robinson <280north.com>
//
Expand Down Expand Up @@ -213,7 +209,7 @@ function _deepEqual(actual, expected, strict, memos) {
}

function isArguments(object) {
return Object.prototype.toString.call(object) == '[object Arguments]';
return Object.prototype.toString.call(object) === '[object Arguments]';
}

function objEquiv(a, b, strict, actualVisitedObjects) {
Expand Down Expand Up @@ -299,7 +295,7 @@ function expectedException(actual, expected) {
return false;
}

if (Object.prototype.toString.call(expected) == '[object RegExp]') {
if (Object.prototype.toString.call(expected) === '[object RegExp]') {
Copy link
Member

Choose a reason for hiding this comment

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

Could we import objectToString from internal/util at the top and just call it here? And if so, would that be more readable?

Copy link
Member Author

Choose a reason for hiding this comment

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

I could go either way on it. (Let me know if you feel strongly one way or the other.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Importing it is probably better. If we ever move toward storing Object.prototype.toString in a variable to prevent tampering in userland code, it would require less updating.

Copy link
Member Author

Choose a reason for hiding this comment

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

Turns out there's already a function internal to assert.js for this.
¯\(ツ)/¯

Copy link
Member Author

Choose a reason for hiding this comment

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

(And I see that function is replaced with the one from internal/util in #11128.)

return expected.test(actual);
}

Expand Down