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

LoosOptions can't print fullhelp and exit if there are required arguments #72

Open
lgsmith opened this issue Feb 4, 2022 · 3 comments

Comments

@lgsmith
Copy link
Contributor

lgsmith commented Feb 4, 2022

Describe the bug
The tool contact_distance.py, while having apparently correct usage of the description and fullhelp strings used to initialize loos options, is unable to print the fullhelp. Instead, it prints:

usage: contact_distance.py [-h] [--fullhelp] -m MODEL --selection SELECTION -t TRAJ
                           [TRAJ ...] [-k SKIP] [-s STRIDE] [--sigma SIGMA]
                           [--radius RADIUS] [--skip_backbone] [--include_h]
                           [--outfile OUTFILE]
contact_distance.py: error: the following arguments are required: -m/--model, --selection, -t/--traj

I think what's happening here is that it's doing its program options logic in a way that doesn't respect whether there's been a fullhelp requested, The parser realizes that a number of required args aren't present and throws the corresponding error, instead of printing full-help and exiting. I'm not exactly sure how to make sure that fullhelp gets printed first. For what it's worth, 'help' has a special status that avoids this issue, so the way argparse handles the -h flag might be the place to start on running this down.

To Reproduce
Steps to reproduce the behavior:

  • contact_distance.py --fullhelp in a functional loos environment with that tool in the path.

Expected behavior
I expected it to print the fullhelp that is saved as that variable name in the source code.

LOOS version and platform
f647665 linux conda install x86

Additional context
none

@agrossfield
Copy link
Member

This is partially but not completely fixed by #76. I got rid of all of the "required" flags on options in LoosOptions, since their presence breaks everything and it doesn't make sense to invoke a specific kind of option and not supply an argument (I suppose I need to add manually checking).

I'm not ready to close this issue yet though because at least a couple of other tools (rare-event-detection.py, perhaps others) still have some required options.

@lgsmith
Copy link
Contributor Author

lgsmith commented Mar 16, 2022

Not exactly sure what you mean by ' it doesn't make sense to invoke a specific kind of option and not supply an argument (I suppose I need to add manually checking).' In the context of normal argparse, required positional arguments can be added to the parser, and it won't screw up your ability to call the tool with the built '-h' flag. Did you mean that the word option doesn't make sense for required args? Because I just thought that was the parlance for command-line parsers that had the capacity for optional args...certainly the C++ version uses that term even though it can represent positional args.

Seems to me like having positionals makes a lot of sense. I've always advocated having positional arguments for required input for a tool. If one finds oneself in a situation with many positional arguments, I usually think it's time to use a config file (JSON is pretty easy for python) instead of having CLI. Or maybe write two tools...I think my limit for different required args is like four or five, but that's kind of a style thing.

Could we just put the huge fullhelp string into the argparser as its help? If a user wants the 'short help', they can still get that by running the tool with no input...

@agrossfield
Copy link
Member

Required arguments break the "no arguments is equivalent to -h" behavior, so we need to root them out everywhere and either replace them with positionals or do some other checking. This will require some testing, which is why I haven't closed this issue yet.

If you look at the commit, what I changed was some of the methods in LoosOptions, eg modelSelectionOptions, that defined options with the required flag. The idea was that if you were doing it that way, those options had to be supplied or the program should exit, but unfortunately the way argparse is implemented that breaks help. I think modelSelectionOptions should perhaps be deprecated in favor of modelSelectionPositionalOptions. The alternative is that the tool itself has to check, which defeats the goal of the pre-packaged options in the first place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants