search

sharkdp / hexyl

A command-line hex viewer
Apache License 2.0
8.92k stars 227 forks source link

Invalid values for `--no-squeezing` should print an error. #109

Closed eth-p closed 3 years ago

eth-p commented 3 years ago

Any value provided to --no-squeezing using the --long=value argument format is treated as true. This means that --no-squeezing=false and --no-squeezing=default will disable squeezing, which might come as a surprise to someone who was expecting those values to retain the default behavior.

It's not likely to happen to an everyday user, but it can happen when writing scripts. In my case, it was while writing a test harness.

Note: I'm doing a class assignment regarding creating tests (from scratch) for an open-source command line program, and I chose hexyl for it. You might have a couple more of these on the way, depending on what the assignment brings to light :)

sharkdp commented 3 years ago

Oh, I didn't even know that clap would allow this as valid command line syntax. --no-squeezing does not take an argument.

eth-p commented 3 years ago

Oh, I didn't even know that clap would allow this as valid command line syntax. --no-sequeezing does not take an argument.

Fun quirks of many argument parsers: if it doesn't take a value, you can sometimes force it to take one. As for what clap does with it... I guess it either truncates it or saves the value anyways?

A way to improve handling of it could be by checking if the forced value can be parsed as a bool, and using that as the value. If not and it's empty, use true. Otherwise, print an error?

Or simply just printing an error for any nonempty value would work, too.

sharkdp commented 3 years ago

Fun quirks of many argument parsers: if it doesn't take a value, you can sometimes force it to take one

Indeed. Seems like a known issue: https://github.com/clap-rs/clap/issues/1543

A way to improve handling of it could be by checking if the forced value can be parsed as a bool, and using that as the value. If not and it's empty, use true. Otherwise, print an error?

I'd prefer not to add any special handling for this. The documentation clearly says that this option does not take a value. The fact that clap allows this is weird, but not critical IMO. We also wouldn't want to add code to make sure that hexyl --help=no is disallowed.

eth-p commented 3 years ago

We also wouldn't want to add code to make sure that hexyl --help=no is disallowed.

That is an extremely good point. And if it isn't an intentional behavior in clap, then let's just leave it up to be fixed by a future clap update. I'll close this issue.