8000 assert: apply minor refactoring by Trott · Pull Request #11511 · nodejs/node · GitHub
  • [go: up one dir, main page]
    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

    Jump to bottom
    Closed
    Trott wants to merge 1 commit into from
    Closed

    assert: apply minor refactoring #11511

    Trott wants to merge 1 commit into from
    +2 −6

    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
    }

    if (Object.prototype.toString.call(expected) == '[object RegExp]') {
    if (Object.prototype.toString.call(expected) === '[object RegExp]') {
    Copy link
    Member
    @addaleax addaleax Feb 22, 2017

    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
    @Trott Trott Feb 23, 2017

    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
    @cjihrig cjihrig Feb 23, 2017

    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
    @Trott Trott Feb 25, 2017

    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
    @Trott Trott Feb 25, 2017

    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 <thechargingvolcano@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
    @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
    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 <thechargingvolcano@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
    @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 <thechargingvolcano@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
    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 <thechargingvolcano@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
    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 <thechargingvolcano@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
    @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 <thechargingvolcano@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
    @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

    @jasnell jasnell jasnell 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

    +2 more reviewers

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

    @thefourtheye thefourtheye thefourtheye approved these changes

    Reviewers whose approvals may not affect merge requirements

    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
    0