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

Rework/Enhance help and usage #187

Closed
wants to merge 4 commits into from

Conversation

ndevenish
Copy link
Contributor

Generally reworked help output to be a little more like what I expect (or, at least, like python's argparse)

  • Added metavars. If set, then will be used in help for the value placeholder for options, or the placeholder alone for regular positional arguments.
  • Help option sections are indented by two spaces, and comma-separated. They now show metavar (this means that an option with value now shows as --option VALUE Description of Option instead of --option Description ....
  • Usage section now enumerates options, instead of just [options]. Added ArgumentParser::usage() to retreive this string alone (in case you wanted to, say, print usage if arguments incorrectly passed).
  • Not showing [default: ...] for implicit values. This seemed unusual. It might be better to rework the help system to recognise a placeholder (even python's %(default)s) rather than try to magically get this right.
  • I've refreshed the README with the updated strings from this approach.

I guess the biggest holes are:

  • Multiple argument support is a little underrepresented compared to the full semantics of python's argparse. I don't know how well the approach to multiple arguments in this package matches the semantics of the python package.
  • I haven't written tests for this because the help output doesn't seem to be tested. I'm not opposed to writing some.

Otherwise this... might not be how you want things. If you like these changes I'm happy to do some amount of reworking to get it up to scratch for merging - these changes work for my purposes, but it could be that you wanted all of this done some other way. I thought I'd at least try to polish them up if it was useful ;)


A slightly expanded version of the README second example:

  argparse::ArgumentParser program("main");
  program.add_argument("thing").help("Thing to use.").metavar("THING");
  program.add_argument("--member").help("The alias for the member to pass to.").metavar("ALIAS");
  program.add_argument("--verbose").default_value(false).implicit_value(true);
  program.add_description("Forward a thing to the next member.");
  program.add_epilog("Possible things include betingalw, chiz, and res.");

Gives:

% ./main --help
Usage: main [-h] [--member ALIAS] [--verbose] THING

Forward a thing to the next member.

Positional arguments:
  THING         	Thing to use.

Optional arguments:
  -h, --help    	shows help message and exits
  -v, --version 	prints version information and exits
  --member ALIAS	The alias for the member to pass to.
  --verbose

Possible things include betingalw, chiz, and res.

This will show in the help as e.g.
    --option METAVAR    Description of option help
Also, add ArgumentParser.usage() method to get just the usage line.
- Indent lists of arguments
- Separate argument names with ", "
- Don't show "default" for implicit arguments
Copy link
Contributor

@skrobinson skrobinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, I like the feature. Thank you for your work.

I'd like to see a metavar-specific example added to README. This is a nice feature that should be pointed out in the docs.

usage << "[";
}
usage << longest_name;
const std::string metavar = m_metavar.size() > 0 ? m_metavar : "VAR";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use !m_metavar.empty(). There are three of these.

return 2 + m_metavar.size();
} else {
// Indent and space-separated
return 2 + names_size + (m_names.size() - 1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the else after return.

std::copy(std::begin(argument.m_names), std::end(argument.m_names),
std::ostream_iterator<std::string>(name_stream, " "));
name_stream << " "; // indent
if (argument.is_positional(argument.m_names.front())) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is_positional is a static member function and does not need an instance.

@ndevenish
Copy link
Contributor Author

Hi, just a note that I'm happy to make these changes and rework the README, I've just been a little busy and won't be able to get to it for a couple of weeks 😅

p-ranav added a commit that referenced this pull request Sep 22, 2022
METAVAR, Improved help, Samples (Based on #187)
@p-ranav
Copy link
Owner

p-ranav commented Sep 22, 2022

Refactored and merged as part of #206, including changes requested by @skrobinson. Thanks to both of you!

@p-ranav p-ranav closed this Sep 22, 2022
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

Successfully merging this pull request may close these issues.

3 participants