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

optparse: add flags fields to option and subcommand structures #910

Merged
merged 11 commits into from Nov 21, 2016

Conversation

Projects
None yet
5 participants
@grondo
Copy link
Contributor

grondo commented Nov 19, 2016

As suggested by @chu11, add new flags fields to struct optparse_option and struct optparse_subcommand now, before there are many more call sites to update.

The struct optparse_option change was actually very painless, since almost all callers used designated initializers, most use cases did not even have to change. I did, however, remove the special case list_argument (i.e. has_arg == 3) extension and instead made this an option flag OPTPARSE_OPT_AUTOSPLIT, which is a nice improvement over the previous, more messy approach.

The addition of subcommand flags was a bit more work, and is perhaps questionable since we already had a way to set arbitrary options/flags/attributes of a subcommand with optparse_set (p, ITEM, ...). However it did seem a nice improvement to keep subcommand initializer all together in one place when using an optparse_subcommand table, so I went ahead and did it. I'd appreciate feedback if this was the right decision.

A flag OPTPARSE_SUBCMD_SKIP_OPTS was added that does the same thing as optparse_set (subcmd, OPTPARSE_SUBCMD_NOOPTS, 1) and the one caller (flux hwloc lstopo) was updated to use the flag initializer instead of optparse_set.

In both cases, a *_HIDDEN option was added, since that was a trivial extension and will now be there if we need it.

@grondo grondo added the review label Nov 19, 2016

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 19, 2016

Coverage Status

Coverage increased (+0.02%) to 76.176% when pulling 276a65e on grondo:optparse-flags into a9ff086 on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 19, 2016

Current coverage is 75.84% (diff: 100%)

Merging #910 into master will increase coverage by 3.32%

@@             master       #910   diff @@
==========================================
  Files           159        149     -10   
  Lines         27162      25991   -1171   
  Methods           0          0           
  Messages          0          0           
  Branches          0          0           
==========================================
+ Hits          19699      19714     +15   
+ Misses         7463       6277   -1186   
  Partials          0          0           
Diff Coverage File Path
•••••••••• 100% src/cmd/builtin/hwloc.c
•••••••••• 100% src/cmd/builtin/dmesg.c
•••••••••• 100% src/cmd/builtin/content.c
•••••••••• 100% src/cmd/builtin/help.c
•••••••••• 100% src/cmd/builtin/env.c
•••••••••• 100% src/cmd/builtin/proxy.c
•••••••••• 100% src/cmd/builtin/nodeset.c
•••••••••• 100% src/cmd/builtin/version.c
•••••••••• 100% src/common/liboptparse/optparse.c
•••••••••• 100% src/cmd/builtin/attr.c

Review all 12 files changed

Powered by Codecov. Last update 55c99a3...59ae3d4

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Nov 19, 2016

I like the change to has_arg, it appears to be much clearer now. Outside of the few nits I noticed, I saw you added a OPTPARSE_SUBCMD_HIDDEN test but not OPTPARSE_SUBCMD_HIDE. Should we test both?

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Nov 19, 2016

Yeah, more tests should be added, but I wanted feedback on the subcommand changes before cleaning this up as a meege candidate.

Am I just missing the other review notes, or are those in progress?

@@ -146,7 +158,7 @@ optparse_t *optparse_get_parent (optparse_t *p);

/*
* Convenience function like optparse_add_subcommand, additionally
* registering with usage string [usage], documentation blurb [doc] and
* registering with usage string [usage], documentation blurb[doc] and

This comment has been minimized.

@chu11

chu11 Nov 19, 2016

Contributor

was this an accidental delete of a space?

This comment has been minimized.

@grondo

grondo Nov 19, 2016

Author Contributor

Yes, thanks. Accidental commit

store_opts,
},
{ "dropcache",
NULL,
"Drop non-essential entries from local content cache",
internal_content_dropcache,
NULL,
0, NULL,

This comment has been minimized.

@chu11

chu11 Nov 19, 2016

Contributor

nit formatting, should be a separate line like others in this file

This comment has been minimized.

@grondo

grondo Nov 19, 2016

Author Contributor

Good catch, thanks

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Nov 19, 2016

@grondo Ahh, I needed to click the "submit review" button on the "Files Changed" tab. Dunno if that's new or maybe how you guys have things setup. Hopefully you can see the two nits now.

@grondo grondo force-pushed the grondo:optparse-flags branch from 276a65e to b677713 Nov 20, 2016

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 20, 2016

Coverage Status

Coverage increased (+0.02%) to 76.151% when pulling b677713 on grondo:optparse-flags into 3defa97 on flux-framework:master.

@grondo grondo force-pushed the grondo:optparse-flags branch from b677713 to 3df5587 Nov 20, 2016

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 20, 2016

Coverage Status

Coverage increased (+0.03%) to 76.163% when pulling 3df5587 on grondo:optparse-flags into 3defa97 on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Nov 21, 2016

This looks good to me. Rebase on current master and then merge?

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Nov 21, 2016

Yeah, let me check it over once more and rebase

@grondo grondo force-pushed the grondo:optparse-flags branch from 3df5587 to d66d6ba Nov 21, 2016

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 21, 2016

Coverage Status

Coverage increased (+0.03%) to 76.149% when pulling d66d6ba on grondo:optparse-flags into 55c99a3 on flux-framework:master.

@grondo grondo force-pushed the grondo:optparse-flags branch from 86b88b0 to 59ae3d4 Nov 21, 2016

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 21, 2016

Coverage Status

Coverage increased (+0.02%) to 76.145% when pulling 59ae3d4 on grondo:optparse-flags into 55c99a3 on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Nov 21, 2016

Ok, I threw on one more unrelated fix, a typo in codecov.yml for libjson-c was causing that library to be counted in codecov coverage check.

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Nov 21, 2016

LGTM, +1

grondo added some commits Nov 18, 2016

liboptparse: add flags member to optparse_option struct
Add a flags member to struct optparse_option (unused for now).

Resolves #907
liboptparse: remove list_argument in favor of option flag
Remove use of special `has_arg` value of 3 for "autsplit" list argument
and replace with a special option flag OPTPARSE_OPT_AUTOSPLIT.

This is much clearer than the previous approach, and allows autosplit
be used with either require or optional arguments. Also, the value can
be passed directly to GNU getopt() without special cases.
liboptparse: support undocumented options
Support undocumented options with OPTPARSE_OPT_HIDDEN flag.
test/optparse: test OPTPARSE_OPT_HIDDEN
Add tests for OPTPARSE_OPT_HIDDEN flag.
liboptparse: Add flags to subcommand initialization
Add a flags parameter to struct optparse_subcommand and
optparse_reg_subcommand() for future use. Update all callers.

No flags are used at this time.
liboptparse: add flag OPTPARSE_SUBCMD_SKIP_OPTS
Allow optparse subcommand property "skip_options" to be set during
subcommand registration via a new flag: OPTPARSE_SUBCOMMAND_SKIP_OPTS.

This flag will disable options processing for the subcommand, instead
passing argc, argv directly to the subcommand callback.

Use of this flag is the same as

 optparse_set (subcmd, OPTPARSE_SUBCMD_NOOPTS, 1);
cmd/builtin: switch hwloc-lsotopo to use subcmd flags
Switch hwloc-lstopo subcommand to initialize SUBCMD_NOOPTS via
subcommand flags instead of a separate call to optparse_set().
liboptparse: allow hidden subcommands
Allow undocumented subcommands through use of OPTPARSE_SUBCMD_HIDDEN
flag, or OPTPARSE_SUBCMD_HIDE optparse_set() option. Subcommands with
this flag set will not appear in the help/usage output of the parent
command.
test/optparse: test hidden subcommands
Add simple test for hidden optparse subcommands.
test/optparse: test usage with long only option
Ensure expected usage output with optparse long-only options
codecov: ignore libjson-c
Fix typo in libjson-c path so this imported code is properly
ignored in codecov analysis.

@grondo grondo force-pushed the grondo:optparse-flags branch from 59ae3d4 to 625e6d1 Nov 21, 2016

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 21, 2016

Coverage Status

Coverage decreased (-0.03%) to 76.145% when pulling 625e6d1 on grondo:optparse-flags into 43cc545 on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Nov 21, 2016

Ready?

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Nov 21, 2016

Yes.

@garlick garlick merged commit 2cca060 into flux-framework:master Nov 21, 2016

4 checks passed

codecov/patch 100% of diff hit (target 72.57%)
Details
codecov/project 75.84% (+3.26%) compared to 43cc545
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.03%) to 76.145%
Details

@garlick garlick removed the review label Nov 21, 2016

@grondo grondo deleted the grondo:optparse-flags branch Nov 22, 2016

@grondo grondo referenced this pull request Nov 28, 2016

Closed

Create 0.6.0 release notes #916

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.