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

Let long/short argument options be atoms (or error out) #22

Open
eproxus opened this issue Mar 12, 2022 · 2 comments
Open

Let long/short argument options be atoms (or error out) #22

eproxus opened this issue Mar 12, 2022 · 2 comments
Assignees
Labels
enhancement New feature or request

Comments

@eproxus
Copy link

eproxus commented Mar 12, 2022

It would be nice if the short and long options for arguments could take atoms, or at least throw errors if they won't. I had this configuration:

#{
    arguments => [
        #{name => backgrounds, short => b, long => backgrounds, type => binary, default => "0..8"}
    ]
}

Calling the escript CLI gave this output:

$ ./tool
usage: colortool
$ ./tool -b foo
error: tool: unrecognised argument: -b
usage: tool

Of course, it works if I change it to $b. This one took a long time to debug 😄

If they should take atoms, I think short should only take one letter atoms and error out otherwise, and long should take any string atom and automatically prepend it with "-" so that the default flag format is --flag (since most UNIX CLIs follow that).

@max-au max-au added the enhancement New feature or request label Mar 13, 2022
@max-au max-au self-assigned this Mar 13, 2022
@max-au max-au added bug Something isn't working and removed enhancement New feature or request bug Something isn't working labels Mar 13, 2022
@max-au
Copy link
Owner

max-au commented Mar 13, 2022

I dug into this specific issue and found that argparse already throws when short is not a printable character, or long is not a printable string.
Now what's been happening with cli, it was catching this exception, and printing this to Erlang Logger:

 WARNING REPORT==== 13-Mar-2022::11:14:04.469019 ===
Error calling bare:cli(): error:{argparse,
                                 {invalid_option,
                                  ["erl"],
                                  backgrounds,long,"must be a string"}}
[{argparse,fail,1,
           [{file,"/home/max-au/git/max-au/argparse/src/argparse.erl"},
            {line,834}]},
 {argparse,'-validate_command/2-lc$^0/1-0-',2,
           [{file,"/home/max-au/git/max-au/argparse/src/argparse.erl"},
            {line,881}]},
 {argparse,validate_command,2,
           [{file,"/home/max-au/git/max-au/argparse/src/argparse.erl"},
            {line,881}]},
 {cli,'-discover_commands/2-fun-0-',5,
      [{file,"/home/max-au/git/max-au/argparse/src/cli.erl"},{line,128}]},
 ...

This is a deliberate decision, to keep running cli even when some command has invalid description, and just log instead. The reason is, we often have dozens and even hundreds of modules exporting cli, and it happens that some commands may have a broken description, or something else broken. Therefore we cannot fail the entire cli, and have to ignore the malformed command.
I understand that this behaviour is not exactly useful for smaller projects. What I think of, there is warn option available for cli:run/2, that can be set to warn (default meaning "print the discovered issue to logger") and suppress (silently ignore). I can add another value, halt, that would mean "print the problem to stdout and halt the VM".

@max-au
Copy link
Owner

max-au commented Mar 13, 2022

Another clue is given by Dialyzer, it immediately warns against atoms set as short or long attributes. It might be enough signal to figure out the invalid argument description.

@max-au max-au added the enhancement New feature or request label Mar 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants