search

BurntSushi / cargo-benchcmp

A small utility to compare Rust micro-benchmarks.
The Unlicense
343 stars 21 forks source link

Allow running as either `cargo benchcmp` or `cargo-benchcmp` #21

Closed fitzgen closed 8 years ago

fitzgen commented 8 years ago

Fixes #20.

Had to change the docopt usage string. Unsure if there is a better way.

fitzgen commented 8 years ago

Had to change the docopt usage string. Unsure if there is a better way.

I suppose another way would be to see if the first argument ends with "cargo-benchcmp" and the second argument is not "benchcmp" and then automatically insert "benchcmp" as the second argument.

Apanatshka commented 8 years ago

I would prefer a method that doesn't change the usage string. I find the docs confusing without mentioning cargo in there, given that the usage should be cargo benchcmp or cargo-benchcmp...

Apanatshka commented 8 years ago

Ok, consider me blind. I missed your second comment completely. That way, adding in benchcmp, sounds like a nicer approach to me.

fitzgen commented 8 years ago

@BurntSushi have an opinion about the above?

BurntSushi commented 8 years ago

I'd like for the usage string to reflect how the user should use the tool, which is cargo benchcmp. However, cargo-benchcmp should also work. I think this means we should look at argv[0], and if it's basename is cargo-benchcmp, insert benchcmp at argv[1] (and move all other arguments down). I think that solves this without changing the usage string, right?

fitzgen commented 8 years ago

I think this means we should look at argv[0], and if it's basename is cargo-benchcmp, insert benchcmp at argv[1](and move all other arguments down). I think that solves this without changing the usage string, right?

Yep, this is what I was trying to suggest as an alternative in https://github.com/BurntSushi/cargo-benchcmp/pull/21#issuecomment-251000816

I'll implement this and then comment again once I've updated the PR.

fitzgen commented 8 years ago

@BurntSushi I've force pushed a new version that adds "benchcmp" when invoked as "cargo-benchcmp" rather than removing "benchcmp" when invoked as "cargo benchcmp", which lets us keep the original USAGE string.

BurntSushi commented 8 years ago

Hmm... Looks like Windows build is failing, but it appears unrelated to this PR.

@fitzgen Thanks so much! :-)

fitzgen commented 8 years ago

Thank you!

BurntSushi commented 8 years ago

My apologies, but I had to back this change out because of #26.

I'm not necessarily opposed to merging another fix for this, but I'll have to insist that it be well tested.

fitzgen commented 8 years ago

No problem -- not sure if this is really worth jumping through hoops for.