FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

internal: use internal/errors by BridgeAR · Pull Request #13829 · nodejs/node · GitHub

/ node Public

internal: use internal/errors - #13829

Closed
BridgeAR wants to merge 1 commit into
nodejs:masterfrom
BridgeAR:refactor-internal-errors
ConversationCommits (1)ChecksFiles changed
Closed

internal: use internal/errors#13829
BridgeAR wants to merge 1 commit into
nodejs:masterfrom
BridgeAR:refactor-internal-errors

Conversation

BridgeAR commented Jun 20, 2017 •
edited by refack
Loading

Copy link
Copy Markdown
Member

Ref: #11273

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

internal

nodejs-github-bot added child_process Issues and PRs related to the child_process subsystem. fs Issues and PRs related to the fs subsystem / file system. util Issues and PRs related to the built-in util module. labels Jun 20, 2017
BridgeAR commented Jun 20, 2017
View reviewed changes
Comment thread lib/internal/fs.js Outdated

BridgeAR Jun 20, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Shouldn't these be TypeErrors?

refack Jun 20, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I agree. X 2

refack commented Jun 20, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

Ref: #13730
Oh NM. I thought it was your other idea.

refack added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 20, 2017
refack suggested changes Jun 20, 2017
View reviewed changes

refack left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Good, but needs a few tweaks

Comment thread test/parallel/test-child-process-constructor.js Outdated

refack Jun 20, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

{type} should be added.
{message} would be nice.

Comment thread lib/internal/util.js Outdated

refack Jun 20, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The other way around (rename the formal parameter) - https://nodejs.org/api/util.html#util_util_promisify_original

Comment thread lib/internal/fs.js Outdated

refack Jun 20, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I agree. X 2

BridgeAR commented Jun 21, 2017

Copy link
Copy Markdown
Member Author

Comments addressed

refack commented Jun 21, 2017

Copy link
Copy Markdown
Contributor

Better, but all common.expectsError need {type} (since before the RegEx did assert the Type as part of the message).

BridgeAR force-pushed the refactor-internal-errors branch from e57cc6e to 904ad1b Compare June 21, 2017 12:51

BridgeAR commented Jun 21, 2017

Copy link
Copy Markdown
Member Author

Addressed and rebased

refack previously approved these changes Jun 21, 2017
View reviewed changes

refack left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

💯

jasnell mentioned this pull request Jun 21, 2017
Closed
80 tasks

refack commented Jun 21, 2017

Copy link
Copy Markdown
Contributor

Just found #11317.
But IMHO this PR's approach is better

BridgeAR commented Jun 21, 2017

Copy link
Copy Markdown
Member Author

@refack I tried to find a duplicate and I didn't find that one... I am fine to remove the overlapping changes here. I guess that would be best?

BridgeAR commented Jun 21, 2017

Copy link
Copy Markdown
Member Author

Oh, #11317 was closed when I was looking for it. That's why I couldn't find it 😃

refack commented Jun 21, 2017

Copy link
Copy Markdown
Contributor

Oh, #11317 was closed when I was looking for it. That's why I couldn't find it 😃

I don't think flags need a special error code.

refack commented Jun 21, 2017

Copy link
Copy Markdown
Contributor

@refack I tried to find a duplicate and I didn't find that one... I am fine to remove the overlapping changes here. I guess that would be best?

Yep.

jasnell reviewed Jun 22, 2017
View reviewed changes
Comment thread lib/internal/child_process.js Outdated

jasnell Jun 22, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

nit: these need to be lined up.

jasnell reviewed Jun 22, 2017
View reviewed changes
Comment thread lib/internal/util.js Outdated

jasnell Jun 22, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

this change looks entirely unrelated to the internal/errors change... it should likely be in it's own PR

mhdawson Jun 23, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

+1

BridgeAR Jun 23, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think it is pretty much a leftover of a former change. I'll remove it again

BridgeAR Jul 11, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I had another look at it and I changed it to original because the error is thrown with original and not orig. I originally changed the error but @refack suggested to change the variable name instead as the documentation also speaks of original.

jasnell reviewed Jun 22, 2017
View reviewed changes
Comment thread test/parallel/test-child-process-constructor.js Outdated

jasnell Jun 22, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

should go ahead and include the message here too

jasnell reviewed Jun 22, 2017
View reviewed changes
Comment thread lib/internal/fs.js Outdated

jasnell Jun 22, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Having a specific ERR_UNKNOWN_ENCODING error would be better here.

refack Jun 22, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I believe this and the flags assertion are superseded by #11317
(Where I made the opposite argument 😉 #11317 (comment) )

mhdawson Jun 23, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think I agree that sticking to the generic one makes sense, but open discussion on the merits of a more specific one.

jasnell reviewed Jun 22, 2017
View reviewed changes
Comment thread test/parallel/test-fs-open-flags.js Outdated

jasnell Jun 22, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

A more specific error here would likely be better.
ERR_FS_UNKNOWN_FILE_OPEN_FLAG or something similar

refack mentioned this pull request Jul 7, 2017
Merged
4 tasks

refack commented Jul 9, 2017

Copy link
Copy Markdown
Contributor

@BridgeAR now that the other PRs landed, could you rework & rebase. Probably could use the new expectsError(fn, options) in allot of these places.

refack dismissed their stale review July 9, 2017 22:35

Needs rework to use newly landed ERR codes

refack self-assigned this Jul 9, 2017
BridgeAR force-pushed the refactor-internal-errors branch from 904ad1b to 9949174 Compare July 11, 2017 00:25

BridgeAR commented Jul 11, 2017

Copy link
Copy Markdown
Member Author

Rebased

refack commented Jul 11, 2017

Copy link
Copy Markdown
Contributor

Ping @jasnell @mhdawson

mhdawson approved these changes Jul 11, 2017
View reviewed changes

mhdawson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

refack commented Jul 11, 2017

Copy link
Copy Markdown
Contributor

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

refack mentioned this pull request Jul 15, 2017
Closed
BridgeAR force-pushed the refactor-internal-errors branch from 9949174 to 16db827 Compare July 18, 2017 17:33

BridgeAR commented Jul 18, 2017

Copy link
Copy Markdown
Member Author

Rebased. PTAL

refack commented Jul 19, 2017

Copy link
Copy Markdown
Contributor

Re: #13937 (comment) @nodejs/ctc

mcollina commented Jul 19, 2017

Copy link
Copy Markdown
Member

Can you rebase? This is conflicting with master.

internal: use internal/errors
44d655e 
In addition refactor common.throws to common.expectsError
BridgeAR force-pushed the refactor-internal-errors branch from 16db827 to 44d655e Compare July 22, 2017 21:18

BridgeAR commented Jul 22, 2017

Copy link
Copy Markdown
Member Author

There is not much left of this after a couple of rebases as there were other PRs that got merged that did the same thing. This is happening very frequently with the internal/errors and I am wondering how it's possible to prevent these duplicates a bit better. I actually stopped opening PRs with ports because of that.

I squashed the commits as they individually did not change a lot anymore.

mcollina approved these changes Jul 22, 2017
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

refack commented Jul 22, 2017

Copy link
Copy Markdown
Contributor

CI: https://ci.nodejs.org/job/node-test-commit/11312/

refack pushed a commit to refack/node that referenced this pull request Jul 22, 2017
lib: tweak use of internal/errors
095357e 
In addition refactor common.throws to common.expectsError

PR-URL: nodejs#13829
Refs: nodejs#11273
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

refack commented Jul 22, 2017

Copy link
Copy Markdown
Contributor

Landed in 095357e

refack closed this Jul 22, 2017
Trott removed the ctc-review label Jul 24, 2017
jasnell mentioned this pull request Oct 30, 2017
Merged
refack removed their assignment Oct 20, 2018
BridgeAR deleted the refactor-internal-errors branch April 1, 2019 23:36
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell left review comments
mcollina mcollina approved these changes
+2 more reviewers
refack refack approved these changes
mhdawson mhdawson approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

child_process Issues and PRs related to the child_process subsystem. fs Issues and PRs related to the fs subsystem / file system. 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.

7 participants

Back | FazBrowse Home | New Git URL