Skip to content

assert: apply minor refactoring #11511

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

Closed
Trott 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]') {

@addaleax addaleax Feb 22, 2017

Copy link
Copy Markdown
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?

@Trott Trott Feb 23, 2017

Copy link
Copy Markdown
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.)

@cjihrig cjihrig Feb 23, 2017

Copy link
Copy Markdown
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.

@Trott Trott Feb 25, 2017

Copy link
Copy Markdown
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.
¯\(ツ)/¯

@Trott Trott Feb 25, 2017

Copy link
Copy Markdown
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