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

Detect -strip and print a more helpful error message #550

Closed
FRex opened this issue Aug 11, 2023 · 9 comments
Closed

Detect -strip and print a more helpful error message #550

FRex opened this issue Aug 11, 2023 · 9 comments
Labels
T-Docs Issues for adding or improving documentation

Comments

@FRex
Copy link

FRex commented Aug 11, 2023

Before discovering oxipng I used optipng so I'm used to single dash option -strip all.

oxipng -strip is equal to oxipng -s -t rip so it produces an error message that "rip" is not a valid number of threads.

Despite CLI, GNU and Linux experience the error still confuses me for a second or two because I'm so used to optipng.

I assume it's even more confusing to others who are used to optipng or have less experience.

This option is particularly tricky because invocations optipng -o max -strip all *.png and oxipng -o max --strip all *.png look and do exactly the same in both programs and differ only by extra dash.

I propose checking args for -strip before calling get_matches_from(std::env::args()) to print an error message explaining long options with double dash.

Another solution would be aliasing -strip to --strip if the args parsing library allows it (I don't know if it does).

I can prepare this as a PR if you wish.

@ace-dent
Copy link

I can confirm: for seasoned users of oxipng, the handling of parameters is 'unexpected'... #539 ...

@FRex
Copy link
Author

FRex commented Aug 11, 2023

To me it's all fully expected (see below). The only unfortunate trap here is that two PNG optimizers with same options o and strip you want 99% of the time differ in this one character. The invocation is the same except for extra dash for oxipng (I don't mention -Z since that is too slow for most cases so strip and level max are usually all you add to both programs).

I can also cite one precedent for helping common user mistakes: ripgrep allows specifying dir seprator (most people will want to use / while on Windows instead of default \) but in some shells (including git bash from https://git-scm.com/ even if enclosed in single quotes that usually in bash never expand things), the / is expanded to git root install dir, so this very helpful message was added:

$ rg --path-separator=/ myregexhere
A path separator must be exactly one byte, but the given separator is 21 bytes: C:/Program\x20Files/Git/
In some shells on Windows '/' is automatically expanded. Use '//' instead.

In my own programs I also try to usually be extra helpful (if code can feasibly do it, I don't want to burden a human, the human user is there to complete their desired task, not remember niche details specific to my program they might very rarely use), for example I have one program that requires 2 args: a URL and a file, so I don't enforce which argument is which of them since I can in code figure that out from checking if file exists and seeing http(s):// in URL which is which.


My issue text above was brief to be quick to read, but since I see you @ace-dent are interested in or surprised by option handling in the other issue I will write this longer message:

As I come from more Linux (with GNU userland) CLI experience to me it's fully expected what oxipng does. The GNU two dash long option style is a very common convention in many programming languages args libraries for a long time now (with golang being a very notable outlier using one dash long options), and in GNU command line programs all the way from the 80s or 90s (with exceptions like dd or gcc). I'm not sure if GNU invented it but that is where most people first run into it, probably, like I did, when they use Linux or GNU tools on other OSes.

The benefit (as you saw in the other issue) is that you don't run out of letters for short options, scripts can look clearer, short options can still be grouped and not be confused for long options. It also merges nicely with convention to use -- to end option list, since in a way that's an empty long option (empty element as sentinel to mark end of list is a common thing in some languages).

The downside is that newer users or experienced users more familiar with DOS, Windows, non-GNU BSD and so on might be surprised. The fact that many common tools (even gcc and clang) don't adhere to this might confuse new users who are learning CLI at the same time as C (common in university setting, happened to me).

Windows especially is a trap since it's a popular OS, with many less experienced and less technical or still learning or underage users (a child learning programming might not understand VMs and not dare reinstall Linux onto computer their non-technical parents gave them), with much less CLI emphasis, and all over the place in case of (often closed source, distributed with no docs, just as a binary) tools written in C or C++ with handrolled option parsers, and the option character is technically just a single / (and \ is the dir separator, but / also works for 99% of use cases - another common trap) but many portable tools just use dashes, and some Windows programs accept both single - and / before their long options.

Even more interesting and confusing option is the help option. Depending on OS, convention (or lack of it) and so on, each of: help, -help, --help, -HeLp, -?, /? (and all other capitalizations of word "help") can be a valid ask for program help, and in case of programs with subcommands like docker or git they can also be passed to subcommand, not the main program. But that is less problematic and usually program invoked the wrong will say its help or at least how to get it properly (but some don't, even gcc does not).

@andrews05
Copy link
Collaborator

Hi @FRex
I can see how this can be confusing 😆. However I'm unsure if there's really any benefit in implementing something specific for -strip. Other flags could equally cause confusion, e.g. -out would complain that ut isn't a valid optimisation level.

Oxipng isn't advertised as being a drop-in replacement for Optipng and one would assume users would check the help if they got an error they couldn't figure out. If this is a common problem then certainly we might need to do something but so far I believe you're the first to mention it here. One possibility might be to print (shorter) help when an argument parsing error occurs, though I'm not sure if clap has any options to do that.

@FRex
Copy link
Author

FRex commented Aug 16, 2023

The README says it began as a Rust reimplementation of Optipng so some hand-holding for users coming from it seems reasonable to me.

I'm also not the first to mention it, ace-dent ran into dash amount confusion in their issue (linked above) when using both tools in a script too.

@andrews05
Copy link
Collaborator

andrews05 commented Aug 16, 2023

Sorry, I meant specifically the -strip concern has not been reported before. In a more general sense, I agree the README could be improved to make the differences from Optipng clear.

@andrews05 andrews05 added the T-Docs Issues for adding or improving documentation label Aug 16, 2023
@ace-dent
Copy link

My 2¢ - this is a documentation issue.
a. The Readme implied it's a replacement for optipng, so should be clarified it's not a drop-in replacement.
b. Add more details to use of parameters (specify they are case sensitive).
c. The built-in help parameter works as expected, apart from case 3 below. The use of forward slash \h is only likely if you are a seasoned Windows batch command user. I'd suggest a trivial fix, which is whenever an error is generated we provide the last line of text from case 2, which points user to correct --help.

  1. Works:
oxipng.exe -h
oxipng.exe -help
oxipng.exe --help
  1. Fails but correctly suggests For more information, try '--help'.:
oxipng.exe -?
oxipng.exe --?
oxipng.exe -H
oxipng.exe --H
oxipng.exe -HELP
oxipng.exe --HELP
  1. Fails without suggestion:
oxipng.exe /h
oxipng.exe /help

@TPS
Copy link

TPS commented Aug 20, 2023

Wait, why does oxipng.exe -help work?! That leads to all kinds of bad precedent-based thoughts for this issue.

Just throwing an idea out (don't shoot! 🙆🏾 ): is it worth kicking this whole thing over to a switch-matching library? It fairly reeks of the glob-type issues.

@andrews05
Copy link
Collaborator

@TPS oxipng uses the Rust de-facto standard argument parsing library clap 🙂
The only slightly surprising behaviour here is that -help works (I guess it's handled as a special case). Everything else is currently exactly as expected (at least from a unix point of view).

If you think this is important, I could suggest to clap that they accept /h for help (on windows only, where it isn't confused for a path).

There's nothing oxipng itself can/should do in its own code for any of this, other than improvements to the Readme.

@andrews05
Copy link
Collaborator

The Read Me has been updated with a few clarifications around these issues. Hope it helps 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Docs Issues for adding or improving documentation
Projects
None yet
Development

No branches or pull requests

4 participants