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: remove requirement for option key on long-only options (and other fixes) #768

Merged
merged 11 commits into from Aug 15, 2016

Conversation

Projects
None yet
3 participants
@grondo
Copy link
Contributor

grondo commented Aug 15, 2016

This is a few minor fixes and improvements for optparse:

  • Remove the requirement for a non-printable option key for long-only options. Instead we use the longindex returned by getopt_long to find the option used
  • Fix some issues with usage output line splitting
  • Add tests for long-only options, usage output corner cases, and optional argument options

grondo added some commits Aug 13, 2016

optparse: do not depend on long option key
When a long option is used, do not depend on the long option "key"
returned in `c` by getopt_long() to find the option info structure.
Instead, use the `longindex` parameter if it is valid.

This allows long-only options to use the same key, instead of
arbitrary non-printing characters.
optparse: allow c == 0 in getopt loop
Allow 0 return from getopt_long() in option processing loop, so
that uninitialized `key` for long-only options doesn't break from
loop.
codecov: updates in yaml config
Add the "tree" comment style to codecov PR comments, which should
indicate the difference in coverage for each file that is part of
the PR.

Also add some missing files that should be ignored in code coverage
reports.
test/optparse: add tests for long only options
Add tests to ensure long-only option "keys" can be initialized to zero.
cmd/flux-start: no need for key on long-only options
Remove the .key member of the optparse table for long-only
options. A non-printable key used as identifier is no longer
necessary for long-only options.
test/optparse: Add tests for optional arguments
Add test to ensure options with optional arguments are found
with and without args.
optparse: fix extra char when splitting word
When splitting a word during optparse usage output, an extra
character was being copied into the destination buffer, resulting
in a stray character after the `-` indicating word split.

Reduce the characters copied by 1 to fix.
test/optparse: test usage string split of word
Add a test ensuring usage splits string in the middle of a word
properly when necessary.
optparse: fix COLUMNS check
The check for a number in COLUMNS environment variable was testing
for *p != '\0' instead of *p == '\0', so either an uninitialized
variable was used, or the COLUMNS variable was ignored.

Fix the check so COLUMNS is used by the library.
test/optparse: test usage output with COLUMNS
Ensure usage output adheres to current setting for COLUMNS.
test/optparse: check that usage can be reset
Ensure reset of usage works.

@grondo grondo added the review label Aug 15, 2016

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Aug 15, 2016

Current coverage is 74.72% (diff: 100%)

Merging #768 into master will increase coverage by 0.02%

@@             master       #768   diff @@
==========================================
  Files           145        145          
  Lines         25010      25014     +4   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          18684      18692     +8   
+ Misses         6326       6322     -4   
  Partials          0          0          
Diff Coverage File Path
•••••••••• 100% src/cmd/flux-start.c
•••••••••• 100% src/common/liboptparse/optparse.c

Powered by Codecov. Last update eeb64b1...8780b9e

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Aug 15, 2016

This looks great - ready for merge?

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Aug 15, 2016

Yeah, I don't have anything else for this PR

@garlick garlick merged commit 8c793e1 into flux-framework:master Aug 15, 2016

3 checks passed

codecov/patch 100% of diff hit (target 74.70%)
Details
codecov/project 74.72% (+0.02%) compared to eeb64b1
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@garlick garlick removed the review label Aug 15, 2016

@grondo grondo deleted the grondo:optparse-improve branch Aug 15, 2016

@garlick garlick referenced this pull request Oct 26, 2016

Closed

0.5.0 release notes #879

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.