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

installer: add uninstall subcommand in Rust #3850

Closed

Conversation

kevinkassimo
Copy link
Contributor

Continuation of #3804 , now in Rust.

cli/installer.rs Outdated Show resolved Hide resolved
@kevinkassimo kevinkassimo force-pushed the installer/uninstall_rust branch 2 times, most recently from efa7320 to f9fe477 Compare February 2, 2020 06:16
@ry
Copy link
Member

ry commented Feb 2, 2020

It's a little annoying to have a whole separate subcommand for a niche functionality related to "deno install". I wonder if "deno install --uninstall" wouldn't be better? That doesn't read very nicely, but I think it makes more sense.

We could rename "deno install" to something else. Maybe "deno bookmark". Then you would have "deno bookmark https://foo/bar.js" and "deno bookmark --delete https://foo/bar.js" ? Not sure about that either. Just thinking outloud.

@kevinkassimo
Copy link
Contributor Author

kevinkassimo commented Feb 2, 2020

@ry IMHO to have a flag that reverts the behavior of a command is just SO confusing for the users... the code for uninstall subcommand is simple enough, and it should not add too much maintenance cost.

(I know there are commands like git branch -D, but IMHO it confused me when I first saw it)

Also having a separate command, we can have some clearer behavior like uninstalling multiple command if provided more than one argument, as I have implemented here.

The flag makes sense under more generic subcommand names like bookmark, though I also feel bookmark like a very strange name -- what we generate is indeed a command on its own: can by typed and run write away. bookmark feels more like caching to me, which we already do

@nayeemrmn
Copy link
Collaborator

I've always thought deno install was awkward for what it does, especially when you think about its behaviour on a local module. It definitely acts more like a bookmark, or alias, besides the tacked-on prefetch.

@cknight
Copy link
Contributor

cknight commented Feb 2, 2020

I'd agree that deno install is confusing. I was very surprised the first time I had a look under the covers at what the deno install actually did. Install to me means placing a binary on my system for use, not creating a run script for source code. I think install should be renamed.

@kevinkassimo
Copy link
Contributor Author

How about just deno command or deno cmd (since we are installing commands)? In that case a delete flag -d makes sense.

  • deno command -i ... for adding new commands
  • deno command -d ... for deleting existing commands
  • deno command -l for listing all installed commands

I also have problem with alias since it does feels like creating symlinks or local aliases instead of a command that can be invoked anywhere

@ry
Copy link
Member

ry commented Feb 3, 2020

It would be cool to use "deno bundle" to install programs. On mac/linux we can use shebang to eliminate the shell script entirely.
On Windows we'd have to find somewhere to store the bundle (obvious choice would be as a sibling of the .cmd).

@kevinkassimo
Copy link
Contributor Author

@ry I am actually thinking about something like deno install -b ... which would install the file as bundled (I prefer this over adding flags to deno bundle, since its functionality is closer to install, and the other flags currently on install can be used here)

A minor problem with shebang is that while env -S allows multiple arguments, we cannot store the extra arguments/flags that will be accepted by the script itself (instead of by Deno).

Maybe I can create a subdirectory under installation direction to save all the bundled files there, and still use a basic shell/batch wrapper to invoke them.

@bartlomieju
Copy link
Member

Thanks for the patch @kevinkassimo but seems it superfluous to have another subcommand/flag for something so trivial when you can just rm it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants