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

liboptparse: Support setting alternate usage callback function #929

Merged
merged 4 commits into from Dec 16, 2016

Conversation

Projects
None yet
5 participants
@chu11
Copy link
Contributor

chu11 commented Dec 15, 2016

I thought it was sort of inconvenient to remove the default "help" option and then re-add it back just so we can set an alternate (or no) usage callback function. Since I was duplicating the code in cmd/flux into cmd/flux-kvs, figured could refactor a bit. So I added an option to allow the user to set an alternate usage callback function. The callback can be disabled by setting the callback to NULL.

Also fix one nit along the way.

@garlick garlick added the review label Dec 15, 2016

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 16, 2016

Coverage Status

Coverage increased (+0.03%) to 76.336% when pulling 5ff5060 on chu11:liboptparsealtusage2 into 82e5275 on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 16, 2016

Current coverage is 76.03% (diff: 88.88%)

Merging #929 into master will decrease coverage by <.01%

@@             master       #929   diff @@
==========================================
  Files           149        149          
  Lines         26010      26013     +3   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          19777      19779     +2   
- Misses         6233       6234     +1   
  Partials          0          0          
Diff Coverage File Path
••••• 50% src/cmd/flux.c
•••••••••• 100% src/common/liboptparse/optparse.c

Powered by Codecov. Last update 565ebf1...7ccbcb0

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Dec 16, 2016

I'm a bit worried that this breaks the abstraction that --help is just another option, as you've elevated it to something special. At least, an error should be returned if the "help" option isn't found in optparse_set_usage_fn. An optparse object is not guaranteed to have a --help option.

Also, since this set item is setting the callback for the --help option explicitly, maybe the item should be called OPTPARSE_HELP_FN? Should the --help option and handler be special-cased in other ways as well?

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 16, 2016

Coverage Status

Coverage increased (+0.02%) to 76.321% when pulling 5ff5060 on chu11:liboptparsealtusage2 into 82e5275 on flux-framework:master.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Dec 16, 2016

Ahh, I see about the abstraction aspect now. Thinking about this a tad. Perhaps instead some common/convenience liboptparse functions need to be developed? Minimally, we have two (three w/ future flux-kvs fixups) "help opt" declarations sitting around.

struct optparse_option helpopt = {
      .name = "help", .key = 'h', .usage = "Display this message"
};

It seems like unnecessary duplication.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Dec 16, 2016

Thinking a tad more, perhaps I jumped the gun on this enhancement. With more tools using liboptparse in the future, we can figure out some of the common-ness later on.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Dec 16, 2016

What if instead of special case for --help, you made the new optparse_set item take the option name and a callback function, e.g.

    optparse_err_t e = optparse_set (p, OPTPARSE_OPTION_CB, "help", NULL);

or just a special function

   optparse_cb_f oldcb = optparse_set_option_cb (p, "help", NULL);

Maybe it would be useful to return a copy of the "old" callback, I'm not sure. This removes the special case for --help option, so I'd be happier with it.

Of course, we could also have a function that returns the struct optparse_option * for a given string, then it could be manipulated by the caller at will, but that seems a little bit dangerous.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Dec 16, 2016

I like this one:

    optparse_err_t e = optparse_set (p, OPTPARSE_OPTION_CB, "help", NULL);

I'll try and tweak to do this one instead.

@chu11 chu11 force-pushed the chu11:liboptparsealtusage2 branch from 5ff5060 to ce14f3a Dec 16, 2016

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Dec 16, 2016

just re-pushed with changes. Added error handling so OPTPARSE_BAD_ARG is returned when appropriate and added some extra tests for checking for bad input.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 16, 2016

Coverage Status

Coverage increased (+0.008%) to 76.31% when pulling ce14f3a on chu11:liboptparsealtusage2 into 82e5275 on flux-framework:master.

if (e != OPTPARSE_SUCCESS)
log_msg_exit ("optparse_add_option (\"help\")");
// Disable automatic `--help' in favor of our own usage() from above
optparse_set (p, OPTPARSE_OPTION_CB, "help", NULL);

This comment has been minimized.

@grondo

grondo Dec 16, 2016

Contributor

Though it is almost impossible to have an error, should we check error here just for completeness? (And in case code gets duplicated or used as an example)

This comment has been minimized.

@chu11

chu11 Dec 16, 2016

Author Contributor

Yup, you're right. I was being lazy. New push coming.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Dec 16, 2016

Just one minor comment, otherwise good improvement, thanks!

chu11 added some commits Dec 15, 2016

liboptparse: Support OPTPARSE_OPTION_CB option
Support OPTPARSE_OPTION_CB option to alter an option's callback function.
This is for convenience so a separate struct optparse_option does not have
to be created and a call to remove_option and add_option does not have to
be called.
cmd/flux: Refactor to use OPTPARSE_OPTION_CB
Cleanup initialization by taking advantage of new liboptparse
OPTPARSE_OPTION_CB setting.

@chu11 chu11 force-pushed the chu11:liboptparsealtusage2 branch from ce14f3a to 7ccbcb0 Dec 16, 2016

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 16, 2016

Coverage Status

Coverage decreased (-0.001%) to 76.338% when pulling 7ccbcb0 on chu11:liboptparsealtusage2 into 565ebf1 on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Dec 16, 2016

LGTM! Nice improvement over the old code, thanks.

@grondo grondo merged commit a1fe845 into flux-framework:master Dec 16, 2016

4 checks passed

codecov/patch 88.88% of diff hit (target 76.03%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +12.85% compared to 565ebf1
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.001%) to 76.338%
Details

@grondo grondo removed the review label Dec 16, 2016

@grondo grondo referenced this pull request Mar 28, 2017

Closed

0.7.0 Release Notes #1019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.