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

Closed
wants to merge 1 commit into from
Closed

errors, path: migrate to use internal/errors.js #11319

wants to merge 1 commit into from

Conversation

seppevs
Copy link
Contributor

@seppevs seppevs commented Feb 12, 2017

Migrate path.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)

errors,path

@nodejs-github-bot nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. path Issues and PRs related to the path subsystem. labels Feb 12, 2017
thefourtheye
thefourtheye reviewed Feb 12, 2017
View reviewed changes
lib/path.js Outdated

function assertPath(path) {
if (typeof path !== 'string') {
throw new TypeError('Path must be a string. Received ' + inspect(path));
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'path', 'String');
Copy link
Contributor

Choose a reason for hiding this comment

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

Why typeof path is not passed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The typeof path was not passed before the change. The inspect(path) returned the value, this is not something the ERR_INVALID_ARG_TYPE supports.

@thefourtheye thefourtheye mentioned this pull request Feb 12, 2017
Closed
4 tasks
@thefourtheye
Copy link
Contributor

Related: #11308 also has an implementation for invalidArgType.

@seppevs
Copy link
Contributor Author

seppevs commented Feb 13, 2017

@thefourtheye : yes, it's the same implementation. Just like in #11300

@jasnell jasnell mentioned this pull request Feb 14, 2017
Closed
80 tasks
@joyeecheung joyeecheung added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 14, 2017
@jasnell jasnell added the blocked PRs that are blocked by other issues or PRs. label Apr 5, 2017
@seppevs seppevs force-pushed the path_use_internal_errors branch from 967737a to 9fa0056 Compare May 1, 2017 19:58
@seppevs
Copy link
Contributor Author

seppevs commented May 1, 2017

I've just rebased this.

jasnell
jasnell reviewed May 1, 2017
View reviewed changes
lib/path.js Outdated
@@ -816,7 +816,7 @@ const win32 = {

basename: function basename(path, ext) {
if (ext !== undefined && typeof ext !== 'string')
throw new TypeError('"ext" argument must be a string');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'ext', 'String');
Copy link
Member

Choose a reason for hiding this comment

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

hmm... I'm thinking string (lowercase) would be better on these.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@seppevs seppevs force-pushed the path_use_internal_errors branch from 9fa0056 to a567257 Compare May 3, 2017 06:57
refack
refack suggested changes May 3, 2017
View reviewed changes
Copy link
Contributor

@refack refack left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Need to gourd against changes in messages https://github.com/nodejs/node/blob/master/COLLABORATOR_GUIDE.md#breaking-changes

test/parallel/test-fs-watchfile.js
@@ -16,7 +16,7 @@ assert.throws(function() {

assert.throws(function() {
fs.watchFile(new Object(), common.noop);
}, /Path must be a string/);
}, common.expectsError({code: 'ERR_INVALID_ARG_TYPE', type: TypeError}));
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add an assertion on the message as well?
(Changing the message is semver-major so let's enforce it)

Copy link
Member

Choose a reason for hiding this comment

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

This is a semver-major change, so it's totally fine to change the message.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but let's keep it from regressing.

test/parallel/test-path-parse-format.js
const expectedMessage = common.expectsError({
code: 'ERR_INVALID_ARG_TYPE',
type: TypeError
});
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto.
(You can turn this into a Factory that takes the messages)

test/parallel/test-path.js
@@ -372,7 +372,7 @@ function fail(fn) {

assert.throws(() => {
fn.apply(null, args);
}, TypeError);
}, common.expectsError({code: 'ERR_INVALID_ARG_TYPE', type: TypeError}));
Copy link
Contributor

Choose a reason for hiding this comment

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

Tritto

@fhinkel
Copy link
Member

fhinkel commented May 23, 2017

@seppevs Thanks so much for putting this together. Sorry that it is dragging out for so long due to being a semver-major change.

@jasnell Could you take another look? I think your comment has been addressed.

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

@refack
Copy link
Contributor

refack commented May 23, 2017

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

CI failures are infrastructure related.

mhdawson
mhdawson approved these changes Jun 6, 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

test/parallel/test-path-parse-format.js
{method: 'format', input: [null], message: expectedMessage},
{method: 'format', input: [''], message: expectedMessage},
{method: 'format', input: [true], message: expectedMessage},
{method: 'format', input: [1], message: expectedMessage},
Copy link
Member

Choose a reason for hiding this comment

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

We'll need to agree on whether the tests should check the contents of the message or not, but since have landed tests both with and without, I'm fine with landing as is.

@mhdawson
Copy link
Member

mhdawson commented Jun 6, 2017

@refack I can't quite tell from the comments/discussion if your suggestions have been addressed and you are ok with this landing. It looks like the CI is good and it may be ready to go but I'd like to be sure. Since the CI is also 2 weeks old we probably want to run a new one.

@refack
Copy link
Contributor

refack commented Jun 6, 2017

We'll need to agree on whether the tests should check the contents of the message or not, but since have landed tests both with and without, I'm fine with landing as is.

@mhdawson Like you said, not worth blocking for full message validation.

@mhdawson
Copy link
Member

mhdawson commented Jun 6, 2017

@refack thanks for the quick response. I'll open an issue where we can discuss what we think is the right way to go on the messages and then we can do a second pass to fix up based on what we decide.

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

@mhdawson mhdawson removed the blocked PRs that are blocked by other issues or PRs. label Jun 6, 2017
@mhdawson
Copy link
Member

mhdawson commented Jun 6, 2017

OSX CI failure looks unrelated. Opened this issue to track: #13507

@mhdawson
Copy link
Member

mhdawson commented Jun 6, 2017

CI good so landing.

@mhdawson
Copy link
Member

mhdawson commented Jun 6, 2017

Landed as dcfbbac

@mhdawson mhdawson closed this Jun 6, 2017
mhdawson pushed a commit that referenced this pull request Jun 6, 2017
@seppevs @mhdawson
path: use internal/errors.js
dcfbbac 
PR-URL: #11319
Ref: #11273
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@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

@thefourtheye thefourtheye thefourtheye left review comments

@refack refack refack approved these changes

@fhinkel fhinkel fhinkel approved these changes

@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. path Issues and PRs related to the path subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@seppevs @thefourtheye @fhinkel @refack @mhdawson @jasnell @joyeecheung @nodejs-github-bot