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

process: unify error message from chdir() errors by SirR4T · Pull Request #19088 · nodejs/node · GitHub

/ node Public

process: unify error message from chdir() errors - #19088

Closed
SirR4T wants to merge 2 commits into
nodejs:masterfrom
SirR4T:errorMessageUnification
ConversationCommits (2)ChecksFiles changed
Closed

process: unify error message from chdir() errors#19088
SirR4T wants to merge 2 commits into
nodejs:masterfrom
SirR4T:errorMessageUnification

Conversation

SirR4T commented Mar 2, 2018 •
edited
Loading

Copy link
Copy Markdown
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

Partly fixes #12351

Affected core subsystem(s)

process

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Mar 2, 2018

joyeecheung commented Mar 2, 2018

Copy link
Copy Markdown
Member

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

richardlau reviewed Mar 2, 2018
View reviewed changes
Comment thread test/parallel/test-fs-chdir-errormessage.js Outdated
const process = require('process');
const assert = require('assert');

assert.throws(

richardlau Mar 2, 2018

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

Use common.expectsError instead?

cjihrig approved these changes Mar 2, 2018
View reviewed changes
Comment thread test/parallel/test-fs-chdir-errormessage.js Outdated
'use strict';

require('../common');
const process = require('process');

cjihrig Mar 2, 2018

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

No need to require process

SirR4T commented Mar 2, 2018

Copy link
Copy Markdown
Author

I don't understand why the test is failing, from this ci job link.

richardlau commented Mar 2, 2018

Copy link
Copy Markdown
Member

I don't understand why the test is failing, from this ci job link.

The test was recently modified by #18986 but the CI of that PR also failed the test on a different platform. I'm fairly sure it's unrelated the to the changes in this PR.

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 2, 2018

BridgeAR commented Mar 2, 2018

Copy link
Copy Markdown
Member

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

BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 2, 2018
mcollina approved these changes Mar 2, 2018
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

mcollina commented Mar 2, 2018

Copy link
Copy Markdown
Member

PR with the fix for the broken test: #19093

addaleax commented Mar 2, 2018

Copy link
Copy Markdown
Member

Fwiw, I don’t think changes to the messages of errors that already have had errno-based error codes should have to be semver-major… @nodejs/tsc thoughts?

joyeecheung commented Mar 3, 2018

Copy link
Copy Markdown
Member

Fwiw, I don’t think changes to the messages of errors that already have had errno-based error codes should have to be semver-major… @nodejs/tsc thoughts?

I think @ljharb raised somewhere that we should still treat them as breaking changes until we have assigned .code to all errors thrown from Node.js core, the rationale is if all the errors don't have .code, then the user can not start to fully rely on them.

joyeecheung commented Mar 3, 2018

Copy link
Copy Markdown
Member

That being said, I think we can make an exception for libuv errors like this one because they are pretty old so should be well-recognized in the user land. Not so much about the ERR_* errors though.

ljharb commented Mar 3, 2018

Copy link
Copy Markdown
Member

If the error previously had a code, then it’s less risky to treat a message change as non-major, but the safest bet is waiting until there’s a version of node with zero codeless errors before doing so.

joyeecheung commented Mar 5, 2018

Copy link
Copy Markdown
Member

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

apapirovski commented Mar 6, 2018

Copy link
Copy Markdown
Contributor

Landed in b32bcf7

apapirovski closed this Mar 6, 2018
apapirovski pushed a commit that referenced this pull request Mar 6, 2018
process: unify error message from chdir() errors
b32bcf7 
PR-URL: #19088
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
SirR4T deleted the errorMessageUnification branch March 6, 2018 14:32
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
process: unify error message from chdir() errors
0e9644b 
PR-URL: nodejs#19088
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
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

mcollina mcollina approved these changes
jasnell jasnell approved these changes
addaleax addaleax approved these changes
cjihrig cjihrig approved these changes
joyeecheung joyeecheung approved these changes
richardlau richardlau approved these changes
+1 more reviewer
thefourtheye thefourtheye approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. 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.

ENOENT error message inconsistencies
Back | FazBrowse Home | New Git URL