Skip to content

Conversation

@Trott
Copy link
Member

@Trott Trott commented Oct 14, 2020

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

@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Oct 14, 2020
@Trott
Copy link
Member Author

Trott commented Oct 14, 2020

@iansu @bcoe @cjihrig This can totally wait until after the push to get everything set up for 15.0 and so on--I know there's a lot going on right now--but I was wondering about taking this trivial change a little further and removing the options validation entirely in cases like this when options.recursive is not set and the validated options object is not subsequently used. I might rename validateRmdirOptions() to validateRecursiveRmdirOptions() to make it clear that the only options validated are ones used with the recursive setting (recursive, maxRetries, and retryDelay) and skip it here and in other places where the validated options aren't used.

Another alternative is to keep the validation but warn the user when those other two options are set and recursive is not.

Mentioning that in case I'm missing/forgetting something that makes that A Bad Idea.

@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 14, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 14, 2020
@nodejs-github-bot

This comment has been minimized.

@iansu
Copy link
Contributor

iansu commented Oct 14, 2020

@Trott I think warning when setting either maxRetries or retryDelay without recursive sounds like a good idea. That seems preferable to skipping validation entirely when recursive is not set.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

PR-URL: nodejs#35642
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
@Trott Trott force-pushed the validate-recursive branch from 5626cef to 7f25fe8 Compare October 16, 2020 13:32
@Trott
Copy link
Member Author

Trott commented Oct 16, 2020

Landed in 7f25fe8

@Trott Trott merged commit 7f25fe8 into nodejs:master Oct 16, 2020
@Trott Trott deleted the validate-recursive branch October 16, 2020 13:32
BethGriggs pushed a commit that referenced this pull request Dec 8, 2020
PR-URL: #35642
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Dec 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fs Issues and PRs related to the fs subsystem / file system.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants