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.

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

errors: migrate lib/console #11340

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
7 participants
@mskec
Contributor

mskec commented Feb 13, 2017

Migrate console.js to use internal/errors.js.

Refs: #11273

cc @jasnell

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)
Show outdated Hide outdated lib/console.js Outdated
Show outdated Hide outdated lib/internal/errors.js Outdated
Show outdated Hide outdated test/parallel/test-console-instance.js Outdated
@mskec

This comment has been minimized.

Show comment
Hide comment
@mskec

mskec Feb 13, 2017

Contributor

Thank you for feedback.
Should I add error to docs? documenting new errors

Contributor

mskec commented Feb 13, 2017

Thank you for feedback.
Should I add error to docs? documenting new errors

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 14, 2017

Member

Yes, please add it to the docs using the pattern seen in the other PRs #11273

Member

jasnell commented Feb 14, 2017

Yes, please add it to the docs using the pattern seen in the other PRs #11273

@jasnell jasnell referenced this pull request Feb 14, 2017

Closed

Tracking Issue: Migrate errors to internal/errors.js #11273

78 of 80 tasks complete
@joyeecheung

This comment has been minimized.

Show comment
Hide comment
@joyeecheung

joyeecheung Feb 14, 2017

Member

This looks like a duplicate of #11308, I've added that one to the tracking issue because that one is earlier.

Member

joyeecheung commented Feb 14, 2017

This looks like a duplicate of #11308, I've added that one to the tracking issue because that one is earlier.

@mskec

This comment has been minimized.

Show comment
Hide comment
@mskec

mskec Feb 14, 2017

Contributor

@joyeecheung yes unfortunately I didn't see it before I started.
I think this PR is done now, but you decide which one you want.

Contributor

mskec commented Feb 14, 2017

@joyeecheung yes unfortunately I didn't see it before I started.
I think this PR is done now, but you decide which one you want.

@fhinkel

This comment has been minimized.

Show comment
Hide comment
@fhinkel

fhinkel May 23, 2017

Member

@mskec Thanks so much for putting this together. Sorry that it is dragging out for so long due to being a semver-major change. Could you rebase and also squash your commits (I think all the changes should be one commit, right?). Thanks!

Member

fhinkel commented May 23, 2017

@mskec Thanks so much for putting this together. Sorry that it is dragging out for so long due to being a semver-major change. Could you rebase and also squash your commits (I think all the changes should be one commit, right?). Thanks!

@mskec

This comment has been minimized.

Show comment
Hide comment
@mskec

mskec May 24, 2017

Contributor

@fhinkel I rebased and squashed commits. Let me know if there is anything else

Contributor

mskec commented May 24, 2017

@fhinkel I rebased and squashed commits. Let me know if there is anything else

@@ -112,6 +112,8 @@ module.exports = exports = {
// Note: Please try to keep these in alphabetical order
E('ERR_ARG_NOT_ITERABLE', '%s must be iterable');
E('ERR_ASSERTION', (msg) => msg);
E('ERR_CONSOLE_WRITABLE_STREAM',
(name) => `Console expects a writable stream instance for ${name}`);

This comment has been minimized.

@TimothyGu

TimothyGu May 25, 2017

Member

Why not use the %s syntax already used by several other format strings?

@TimothyGu

TimothyGu May 25, 2017

Member

Why not use the %s syntax already used by several other format strings?

This comment has been minimized.

@mskec

mskec May 25, 2017

Contributor

No special reason. I made this PR when there was no other error messages using %s syntax.
Will change if you think that way is better

@mskec

mskec May 25, 2017

Contributor

No special reason. I made this PR when there was no other error messages using %s syntax.
Will change if you think that way is better

@jasnell

This comment has been minimized.

Show comment
Hide comment
@fhinkel

This comment has been minimized.

Show comment
Hide comment
@fhinkel

fhinkel May 27, 2017

Member

Thanks. Landed in 0ecdf29.

Member

fhinkel commented May 27, 2017

Thanks. Landed in 0ecdf29.

@fhinkel fhinkel closed this May 27, 2017

fhinkel added a commit that referenced this pull request May 27, 2017

errors: migrate lib/console
Migrate console.js to use internal/errors.js.

PR-URL: #11340
Ref: #11273
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

@Trott Trott removed the blocked label Jul 11, 2017

@refack refack added this to Done in Error Codes Aug 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment