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.

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

fs: throw rm() validation errors #35602

Closed
wants to merge 1 commit into from
Closed

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

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

@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
Copy link
Contributor

@bcoe bcoe left a comment

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

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

@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
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
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
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
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
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
Development

Successfully merging this pull request may close these issues.

9 participants