-
Notifications
You must be signed in to change notification settings - Fork 36
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
feat: add rm
command
#29
Conversation
I noticed that there is a different algorithm for parsing here https://github.com/denoland/deno_task_shell/blob/2ade977cbe14402376b56f6c929adaf07de8b27d/src/shell/commands/args.rs#L76 Is it better to try to faithufull port [rust remove command](https://github.com/denoland/deno_task_shell/blob/2ade977cbe14402376b56f6c929adaf07de8b27d/src/shell/commands/rm.rs ? (which I just noticed xD) |
Ok I ended up porting the rust code These questions remain though
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks @sigmaSd! I didn't bother adding these previously because it's trivial to just call Deno.remove
, however this is nice to have as a cross platform command for porting code from shell scripts to a deno script.
I'm not sure how to fix the formatting I tried to use dprint --config but it still the same thing
The command is dprint fmt
. I reverted some of the other random formatting changes by using git.
Deno.remove folds force and recursive into one option recursive, is it ok to expose only --recursive ?
That's fine. We can improve it later.
Thanks for picking up the pr @dsherret Do you think is there a way to avoid duplication between this and the rust crate ? I understand wasm can't do IO (without something like wasi) but what about parsing, maybe in this example instead of porting parse_remove_argumens, I should wasm export it ? |
I'm not sure how to fix the formatting I tried to use dprint --config but it still the same thing
Deno.remove folds force and recursive into one option recursive, is it ok to expose only
--recursive
?