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

wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Feb 22, 2017

  • Remove comment referring to the CommonJS Unit Testing 1.0 spec. This
    module is no longer intended to comply with that spec.
  • Remove puzzling "THIS IS NOT TESTED NOR LIKELY TO WORK OUTSIDE V8!"
    comment. No doubt, it made sense at one time.
  • Favor === over == in two places.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

assert

@Trott
assert: apply minor refactoring
7e2de5f 
* Remove comment referring to the CommonJS Unit Testing 1.0 spec. This
  module is no longer intended to comply with that spec.
* Remove puzzling "THIS IS NOT TESTED NOR LIKELY TO WORK OUTSIDE V8!"
  comment. No doubt, it made sense at one time.
* Favor `===` over `==` in two places.
@Trott Trott added the assert Issues and PRs related to the assert subsystem. label Feb 22, 2017
@Trott
Copy link
Member Author

Trott commented Feb 22, 2017

CI: https://ci.nodejs.org/job/node-test-pull-request/6552/

addaleax
addaleax approved these changes Feb 22, 2017
View reviewed changes
lib/assert.js
@@ -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.)

joyeecheung
joyeecheung approved these changes Feb 24, 2017
View reviewed changes
Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

+1 to use require('internal/util').objectToString in case there is another PR to prevent Object.prototype.toString.call() from being tampered with, though it doesn't need to happen in this PR..#11128 do use objectToString though

Trott added a commit to Trott/io.js that referenced this pull request Feb 25, 2017
@Trott
assert: apply minor refactoring
4fe081d 
* Remove comment referring to the CommonJS Unit Testing 1.0 spec. This
  module is no longer intended to comply with that spec.
* Remove puzzling "THIS IS NOT TESTED NOR LIKELY TO WORK OUTSIDE V8!"
  comment. No doubt, it made sense at one time.
* Favor `===` over `==` in two places.

PR-URL: nodejs#11511
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@Trott
Copy link
Member Author

Trott commented Feb 25, 2017

Landed in 4fe081d. Will make sure that #11128 and/or a subsequent PR eliminates the local instance(s) of Object.prototype.toString.call.

@Trott Trott closed this Feb 25, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 25, 2017
@Trott @italoacasas
assert: apply minor refactoring
6bcc841 
* Remove comment referring to the CommonJS Unit Testing 1.0 spec. This
  module is no longer intended to comply with that spec.
* Remove puzzling "THIS IS NOT TESTED NOR LIKELY TO WORK OUTSIDE V8!"
  comment. No doubt, it made sense at one time.
* Favor `===` over `==` in two places.

PR-URL: nodejs#11511
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@italoacasas italoacasas mentioned this pull request Feb 25, 2017
Merged
jasnell pushed a commit that referenced this pull request Mar 7, 2017
@Trott @jasnell
assert: apply minor refactoring
e95d87f 
* Remove comment referring to the CommonJS Unit Testing 1.0 spec. This
  module is no longer intended to comply with that spec.
* Remove puzzling "THIS IS NOT TESTED NOR LIKELY TO WORK OUTSIDE V8!"
  comment. No doubt, it made sense at one time.
* Favor `===` over `==` in two places.

PR-URL: #11511
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
jasnell pushed a commit that referenced this pull request Mar 7, 2017
@Trott @jasnell
assert: apply minor refactoring
92346db 
* Remove comment referring to the CommonJS Unit Testing 1.0 spec. This
  module is no longer intended to comply with that spec.
* Remove puzzling "THIS IS NOT TESTED NOR LIKELY TO WORK OUTSIDE V8!"
  comment. No doubt, it made sense at one time.
* Favor `===` over `==` in two places.

PR-URL: #11511
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
@Trott @MylesBorins
assert: apply minor refactoring
a0c705e 
* Remove comment referring to the CommonJS Unit Testing 1.0 spec. This
  module is no longer intended to comply with that spec.
* Remove puzzling "THIS IS NOT TESTED NOR LIKELY TO WORK OUTSIDE V8!"
  comment. No doubt, it made sense at one time.
* Favor `===` over `==` in two places.

PR-URL: #11511
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
@Trott @MylesBorins
assert: apply minor refactoring
75af859 
* Remove comment referring to the CommonJS Unit Testing 1.0 spec. This
  module is no longer intended to comply with that spec.
* Remove puzzling "THIS IS NOT TESTED NOR LIKELY TO WORK OUTSIDE V8!"
  comment. No doubt, it made sense at one time.
* Favor `===` over `==` in two places.

PR-URL: #11511
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

@jasnell jasnell jasnell approved these changes

@thefourtheye thefourtheye thefourtheye approved these changes

@addaleax addaleax addaleax approved these changes

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

Assignees
No one assigned
Labels
assert Issues and PRs related to the assert subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@Trott @sam-github @jasnell @thefourtheye @addaleax @lpinca @cjihrig @joyeecheung