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: throw rm() validation errors #35602

Closed
wants to merge 1 commit into from
Closed

fs: throw rm() validation errors #35602

wants to merge 1 commit into from

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Oct 11, 2020

This commit updates validateRmOptions() to throw on input validation failures. This is consistent with how Node handles validation in most places across the codebase.

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

@cjihrig
fs: throw rm() validation errors
97ea49b 
This commit updates validateRmOptions() to throw on input
validation failures. This is consistent with how Node handles
validation in most places across the codebase.
@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Oct 11, 2020
@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 11, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 11, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 11, 2020 •
edited by cjihrig
Loading

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

@cjihrig cjihrig added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 11, 2020
bcoe
bcoe reviewed Oct 11, 2020
View reviewed changes
Copy link
Contributor

@bcoe bcoe 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.

If I'm understanding this correctly, parameter validation will be a crasher, but our enforcement of missing paths/file paths will bubble as an error to the callback?

Almost wonder if these should be two different utility methods, like validateRmOptions, validateRmTarget, not a blocker, just thinking out loud about a possible refactor.

@cjihrig
Copy link
Contributor Author

cjihrig commented Oct 12, 2020

If I'm understanding this correctly, parameter validation will be a crasher, but our enforcement of missing paths/file paths will bubble as an error to the callback?

That's right. Node's approach has historically been to throw on programming errors and use the callback for runtime errors.

Almost wonder if these should be two different utility methods, like validateRmOptions, validateRmTarget, not a blocker, just thinking out loud about a possible refactor.

If I was writing this from scratch, I'd do something like that. validateRmOptions() could be shared between the three implementations, and the stat() call could be done in a separate function, or even inlined.

I mostly just wanted to get this change in before the new function goes out in a release.

@Trott
Copy link
Member

Trott commented Oct 12, 2020 •
edited
Loading

I'm surprised this doesn't require a test to be modified somewhere. Might be good to add a test for this.

@cjihrig cjihrig mentioned this pull request Oct 12, 2020
Closed
@BethGriggs BethGriggs added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 13, 2020
@BethGriggs
Copy link
Member

Fast-track? (to ensure we can get it into the last v14.x before LTS (refs: nodejs/Release#567 (comment))

@BethGriggs BethGriggs added the fast-track PRs that do not need to wait for 48 hours to land. label Oct 13, 2020
@github-actions github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 13, 2020
@github-actions
Copy link
Contributor

Landed in ce4ac15...adf8f3d

@github-actions github-actions bot closed this Oct 13, 2020
nodejs-github-bot pushed a commit that referenced this pull request Oct 13, 2020
@cjihrig @nodejs-github-bot
fs: throw rm() validation errors
adf8f3d 
This commit updates validateRmOptions() to throw on input
validation failures. This is consistent with how Node handles
validation in most places across the codebase.

PR-URL: #35602
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
MylesBorins pushed a commit that referenced this pull request Oct 14, 2020
@cjihrig @MylesBorins
fs: throw rm() validation errors
939f8e8 
This commit updates validateRmOptions() to throw on input
validation failures. This is consistent with how Node handles
validation in most places across the codebase.

PR-URL: #35602
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
@MylesBorins MylesBorins mentioned this pull request Oct 14, 2020
Merged
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
@cjihrig @joesepi
fs: throw rm() validation errors
30ba21c 
This commit updates validateRmOptions() to throw on input
validation failures. This is consistent with how Node handles
validation in most places across the codebase.

PR-URL: nodejs#35602
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bcoe bcoe bcoe left review comments

@aduh95 aduh95 aduh95 approved these changes

@richardlau richardlau richardlau approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@lpinca lpinca lpinca approved these changes

@Trott Trott Trott 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. fast-track PRs that do not need to wait for 48 hours to land. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@cjihrig @nodejs-github-bot @Trott @BethGriggs @bcoe @lpinca @joyeecheung @richardlau @aduh95