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

errors,util: migrate to use internal/errors.js #13293

Closed
wants to merge 1 commit into from
Closed

errors,util: migrate to use internal/errors.js #13293

wants to merge 1 commit into from

Conversation

bidipyne
Copy link
Contributor

  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

util.js

ref: #11273

This is my first contribution in node. I read and understood the contribution guidelines, please review and suggest.
@jasnell
Thanks.

@nodejs-github-bot nodejs-github-bot added the util Issues and PRs related to the built-in util module. label May 30, 2017
@targos targos added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 30, 2017
@addaleax addaleax added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label May 30, 2017
@jasnell
Copy link
Member

jasnell commented Jun 1, 2017

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

@jasnell
Copy link
Member

jasnell commented Jun 2, 2017

@mhdawson @fhinkel ... could I ask one of you to please give this a review :-) thank you!

mhdawson
mhdawson approved these changes Jun 2, 2017
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM once --expose-internals is removed.

test/parallel/test-util-inspect.js Outdated
@@ -19,6 +19,7 @@
// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
// USE OR OTHER DEALINGS IN THE SOFTWARE.

// Flag: --expose-internals
Copy link
Member

Choose a reason for hiding this comment

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

I don' think we need --expose-internals

@bidipyne
Copy link
Contributor Author

bidipyne commented Jun 2, 2017 •
edited
Loading

Done as per suggestion. Thanks! @mhdawson

@mhdawson
Copy link
Member

mhdawson commented Jun 2, 2017

@bidipyne thanks :)

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

jasnell pushed a commit that referenced this pull request Jun 2, 2017
@bidipyne @jasnell
errors,util: migrate to use internal/errors.js
1609899 
PR-URL: #13293
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@jasnell
Copy link
Member

jasnell commented Jun 2, 2017

Landed in 1609899

@jasnell jasnell closed this Jun 2, 2017
@jasnell jasnell mentioned this pull request Oct 30, 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

@mhdawson mhdawson mhdawson approved these changes

Assignees
No one assigned
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core. semver-major PRs that contain breaking changes and should be released in the next major version. util Issues and PRs related to the built-in util module.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@bidipyne @jasnell @mhdawson @addaleax @targos @nodejs-github-bot