Combine deno.removeAll into deno.remove #1596
Conversation
|
|
js/remove.ts
Outdated
| */ | ||
| export async function removeAll(path: string): Promise<void> { | ||
| await dispatch.sendAsync(...req(path, true)); | ||
| export async function remove(path: string, recursive = false): Promise<void> { |
There was a problem hiding this comment.
Should we pass an options object { recursive: boolean } instead of recursive = false ?
This makes it easier to extend the interface
At the same time, single boolean will make people confused
README.md
Outdated
|
|
||
| ``` | ||
| curl -L https://deno.land/x/install/install.py | python | ||
| curl -L https://deno.land/x/install/install.sh | bash |
There was a problem hiding this comment.
This is an unrelated change. Please remove
| export function removeSync(path: string): void { | ||
| dispatch.sendSync(...req(path, false)); | ||
| export interface RemoveOption { | ||
| recursive?: boolean; | ||
| } |
There was a problem hiding this comment.
I'm not a huge fan of these Options interfaces with just a single member...
But I think it makes sense to mirror the interface in deno.mkdir(d, { recursive: true })
cc @piscisaureus here is another example of a function which will never have more than one option.
There was a problem hiding this comment.
Nope! An option could be whether to remove readonly files also (not a thing on linux, but kinda important on windows and a thing on os x also)
There was a problem hiding this comment.
A good argument in favor of using options objects, even when they have a single member, is simply how confusing boolean traps are.
There was a problem hiding this comment.
FYI we recently added this to the style guide https://github.com/denoland/deno_std/blob/bef7ba14303997a05cdd1ea0d28ffefeee75c993/README.md#exported-functions-max-2-args-put-the-rest-into-an-options-object
|
It would be better to call it Should we unify the use of an option object for the other functions? For example On the other hand, it should be changed to And finally, should a new issue be opened for this? (Sorry for my English) |
Closes #1584