holmgr / cargo-sweep

A cargo subcommand for cleaning up unused build files generated by Cargo
MIT License
693 stars 31 forks source link

Get rid of `sweep` subcommand #58

Closed CosmicHorrorDev closed 1 year ago

CosmicHorrorDev commented 3 years ago

It was mentioned in #51 that

Also it appears that the --version flag doesn't work:

$ cargo sweep -V
cargo-sweep.exe-sweep
$ cargo sweep --version
cargo-sweep.exe-sweep
$ cargo sweep --version -v
cargo-sweep.exe-sweep

The Why

This is because cargo will still pass the subcommand to a third-party cargo extension which this project worked around by having a matching sweep subcommand which did all the hard work. This meant that the output showed up as cargo-sweep.exe-sweep since it was actually operating on the sweep subcommand which didn't have a version (and also explains the trailing -sweep.

To make things look nicer I followed cargo-criterion's method of having a hidden subcommand argument that eats the sweep subcommand that is passed in.

Possible Issues

This can cause confusing behavior if people invoked the command directly since the command doesn't appear in the help text, but I think this is fine since all the documentation I could see showed invoking the command using cargo.

Alternative Approach

Another technique (which I would be open to switching over to use instead) is from cargo-tarpaulin which does essentially what this library did, but has the version information on the subcommand and overrides the bin name in clap as cargo so that cargo-tarpaulin tarpaulin --version will display cargo-tarpaulin ... instead of cargo-tarpaulin-tarpaulin ...

I would be open to switching the the above technique as well since it seems less confusing if for whatever reason someone directly calls the bin instead of trying to use it through cargo. I just wanted to put this PR out here to get the ball rolling first (and because I only saw the alternate technique while I was writing up this text :sweat_smile:)

CosmicHorrorDev commented 3 years ago

Looks like the CI changes are from some unrelated issue. Also was sifting through the issue tracker and saw that the issue being addressed in this PR was also mentioned in #44

I decided to take a look at more tools just to get an idea of what others do and here's what I've got

Approach 1

Eat the subcommand using a hidden arg that is consumed

Approach 2

Have the bin name appear as cargo and use a subcommand

Approach 3

Fail when trying to be run directly with a message saying to invoke using cargo. Beyond that follow the technique of Approach 2

Approach 4

Mimic the interface when invoking as cargo-tool and cargo tool while hiding the tool subcommand in cargo-tool

Approach 5

Same as Approach 4, but somehow detect when being invoked by cargo so that doing cargo-tool tool fails instead of working

And that exhausts all the cargo commands that I have installed on my computer. So it looks like most commands fit into one of a few categories, the closest one this tool currently fits into is Approach 2 currently. I think my preference would be

  1. Approach 5
  2. Approach 4
  3. Approach 2
  4. Approach 3
  5. Approach 1

Approach 4 and 5 both make the interface appear as users would normally expect (where invoking with cargo-tool and cargo tool yield the same results)

Approach 2 and 3 both at least make what is happening obvious but are both invoked as cargo tool or cargo-tool tool

Approach 1 could cause confusion since there is a hidden arg being used that would be an issue when someone invokes the tool directly

I'll have to look into how Approach 5 works, but it is currently my most preferred

jyn514 commented 1 year ago

This is a really great summary, thank you for writing it up!

I am not a giant fan of 5; I think cargo-sweep on its own is totally fine and we should support it. 4 seems easy to implement, I'm going to put up a PR with those changes shortly. I do notice it prints cargo-sweep instead of cargo sweep, but that seems like a small price to pay to avoid cargo-sweep without a subcommand silently doing nothing.

jyn514 commented 1 year ago

Ugh, this has the same problem I had in cargo-deadlinks - cargo sweep sweep is itself a valid command that means "clean the directory named sweep", and you can't distinguish it from cargo-sweep sweep sweep, which is invalid.

Ok, giving a hard error when the sweep subcommand is missing seems reasonable to me (5 in your list). that's 3, not 5

marcospb19 commented 1 year ago

After the clap 4 migration, here's the output of --version:

$ cargo sweep --version
cargo-sweep-sweep 0.6.2
$ cargo-sweep --version
cargo-sweep 0.6.2
jyn514 commented 1 year ago

currently the output from cargo-sweep -h is

A tool for cleaning unused build files created by Cargo

Usage: cargo-sweep <COMMAND>

Commands:
  sweep  A tool for cleaning unused build files created by Cargo
  help   Print this message or the help of the given subcommand(s)

Options:
  -h, --help     Print help information
  -V, --version  Print version information

which I think corresponds to approach 2. I think it's ok to close this for now.