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.

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,repl: migrate to use internal/errors.js #13299

Closed
wants to merge 1 commit into from

Conversation

sreepurnajasti
Copy link
Contributor

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

lib/internal/repl.js
lib/internal/errors.js

ref: #11273

@nodejs-github-bot nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. repl Issues and PRs related to the REPL subsystem. labels May 30, 2017
E('ERR_IPC_CHANNEL_CLOSED', 'channel closed');
E('ERR_IPC_DISCONNECTED', 'IPC channel is already disconnected');
E('ERR_IPC_ONE_PIPE', 'Child process can have only one IPC pipe');
E('ERR_IPC_SYNC_FORK', 'IPC cannot be used with synchronous forks');
E('ERR_MISSING_ARGS', missingArgs);
E('ERR_PARSE_HISTORY_DATA',
(oldHistoryPath) => `Could not parse history data in ${oldHistoryPath}`);
Copy link
Member

Choose a reason for hiding this comment

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

Just a nit, but it's a bit odd to have one of the new codes use %s while the other uses a template string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jasnell I have used template string. So, do you want me to convert it to %s?

Copy link
Member

Choose a reason for hiding this comment

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

I would switch the ERR_INVALID_REPL_HISTORY one to use a template string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jasnell Done.

@@ -153,13 +154,14 @@ function setupHistory(repl, historyPath, oldHistoryPath, ready) {
if (oldReplJSONHistory) repl.history = JSON.parse(oldReplJSONHistory);

if (!Array.isArray(repl.history)) {
throw new Error('Expected array, got ' + typeof repl.history);
throw new errors.Error('ERR_INVALID_REPL_HISTORY',
Copy link
Member

Choose a reason for hiding this comment

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

This should likely be a TypeError. Since this is already a semver-major change, may as well fix the type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jasnell I too thought on those lines. But to be consistent with previous code, I did in that manner. Now, it is fixed. Thanks.

@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 30, 2017
@@ -153,13 +154,14 @@ function setupHistory(repl, historyPath, oldHistoryPath, ready) {
if (oldReplJSONHistory) repl.history = JSON.parse(oldReplJSONHistory);

if (!Array.isArray(repl.history)) {
throw new Error('Expected array, got ' + typeof repl.history);
throw new errors.TypeError('ERR_INVALID_ARG_TYPE',
typeof repl.history, 'Array');
Copy link
Member

Choose a reason for hiding this comment

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

be sure to line up the arguments appropriate and run make lint

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jasnell Unfortunately, it is not captured in lint test. Now, it's Done.

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

@jasnell
Copy link
Member

jasnell commented Jun 1, 2017

jasnell pushed a commit that referenced this pull request Jun 2, 2017
PR-URL: #13299
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented Jun 2, 2017

Landed in 2822796

@tniessen
Copy link
Member

Too late, but doesn't LGTM. ERR_INVALID_REPL_HISTORY doesn't appear to be used at all. Please review #13733.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core. repl Issues and PRs related to the REPL subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants