fs: throw rm() validation errors - #35602
Conversation
Sorry, something went wrong.
This commit updates validateRmOptions() to throw on input validation failures. This is consistent with how Node handles validation in most places across the codebase.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
That's right. Node's approach has historically been to throw on programming errors and use the callback for runtime errors.
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. |
Sorry, something went wrong.
|
I'm surprised this doesn't require a test to be modified somewhere. Might be good to add a test for this. |
Sorry, something went wrong.
|
Fast-track? (to ensure we can get it into the last v14.x before LTS (refs: nodejs/Release#567 (comment)) |
Sorry, something went wrong.
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>
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>
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>
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