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

fs: fix error codes for fs.cp #41106

Merged
merged 3 commits into from
Dec 10, 2021
Merged

fs: fix error codes for fs.cp #41106

merged 3 commits into from
Dec 10, 2021

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Dec 7, 2021 •
edited
Loading

The context passed into this error must have .code, .syscall and
.message.

node/lib/internal/errors.js

Lines 215 to 216 in 36c0ac0

// The context passed into this error must have .code, .syscall and .message,
// and may have .path and .dest.

Maybe it'd worth adding a ESLint rule to catch this kind of mistakes.

Fixes: #41104

@aduh95
fs: fix error codes for fs.cp
59c80a2 
The context passed into this error must have `.code`, `.syscall` and
`.message`.

Fixes: nodejs#41104
@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Dec 7, 2021
richardlau
richardlau reviewed Dec 7, 2021
View reviewed changes
Copy link
Member

@richardlau richardlau left a comment

Choose a reason for hiding this comment

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

Tests?

@aduh95
Copy link
Contributor Author

aduh95 commented Dec 7, 2021 •
edited
Loading

Tests?

This only affects the error message, which is usually something we don't test. Or should I add one in test/message anyway?

EDIT: I've added a linter rule instead, PTAL.

@aduh95 aduh95 added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Dec 7, 2021
@richardlau

This comment has been minimized.

Sign in to view
@richardlau
Copy link
Member

Tests?

This only affects the error message, which is usually something we don't test. Or should I add one in test/message anyway?

Does the .code property being added not make it through to the user code outside of the message?

Ah right, I see now. nvm.

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Dec 7, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 7, 2021
@nodejs-github-bot
Copy link
Collaborator

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

BridgeAR
BridgeAR approved these changes Dec 7, 2021
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

LGTM. Checking a single error message might be good for the case that the SystemError implementation would change.

@github-actions github-actions bot mentioned this pull request Dec 8, 2021
Open
47 tasks
@github-actions github-actions bot mentioned this pull request Dec 9, 2021
Open
46 tasks
@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Dec 10, 2021
Open
48 tasks
@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 10, 2021
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 10, 2021
@nodejs-github-bot nodejs-github-bot merged commit 2f60225 into nodejs:master Dec 10, 2021
@nodejs-github-bot
Copy link
Collaborator

Landed in 2f60225

This was referenced Dec 11, 2021
Open
Open
Open
danielleadams pushed a commit that referenced this pull request Dec 13, 2021
@aduh95 @danielleadams
fs: fix error codes for fs.cp
9232435 
The context passed into this error must have `.code`, `.syscall` and
`.message`.

Fixes: #41104

PR-URL: #41106
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@github-actions github-actions bot mentioned this pull request Dec 14, 2021
Open
46 tasks
danielleadams pushed a commit that referenced this pull request Dec 14, 2021
@aduh95 @danielleadams
fs: fix error codes for fs.cp
46108f8 
The context passed into this error must have `.code`, `.syscall` and
`.message`.

Fixes: #41104

PR-URL: #41106
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@danielleadams danielleadams mentioned this pull request Dec 14, 2021
Merged
danielleadams pushed a commit that referenced this pull request Jan 31, 2022
@aduh95 @danielleadams
fs: fix error codes for fs.cp
8f282c0 
The context passed into this error must have `.code`, `.syscall` and
`.message`.

Fixes: #41104

PR-URL: #41106
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 31, 2022
@aduh95 @danielleadams
fs: fix error codes for fs.cp
4c53783 
The context passed into this error must have `.code`, `.syscall` and
`.message`.

Fixes: #41104

PR-URL: #41106
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
@aduh95 @Linkgoron
fs: fix error codes for fs.cp
c8c3c65 
The context passed into this error must have `.code`, `.syscall` and
`.message`.

Fixes: nodejs#41104

PR-URL: nodejs#41106
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
@aduh95 @danielleadams
fs: fix error codes for fs.cp
eaab8d0 
The context passed into this error must have `.code`, `.syscall` and
`.message`.

Fixes: #41104

PR-URL: #41106
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardlau richardlau richardlau left review comments

@BridgeAR BridgeAR BridgeAR approved these changes

@lpinca lpinca lpinca approved these changes

@Trott Trott Trott approved these changes

@evanlucas evanlucas evanlucas approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

fs.cp throws error with message 'cp returned undefined'
7 participants
@aduh95 @richardlau @nodejs-github-bot @evanlucas @Trott @lpinca @BridgeAR