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

Add support for -h arg #128

Merged
merged 3 commits into from
Jan 27, 2017
Merged

Add support for -h arg #128

merged 3 commits into from
Jan 27, 2017

Conversation

gerdreiss
Copy link
Contributor

Offering -h option by adding an option with name "help" and short name "h" and letting Scallop handle the printing of the help text

@hrj
Copy link
Owner

hrj commented Jan 27, 2017

Thanks @gerdreiss ! The code seems to work fine but the tests are failing.

I had a quick look but couldn't figure out what's going on.

@hrj
Copy link
Owner

hrj commented Jan 27, 2017

One thing to note, specifying the -h command to scallop is not required. Scallop is adding it automatically, by default.

So the only question is why the test is failing.

@gerdreiss
Copy link
Contributor Author

gerdreiss commented Jan 27, 2017

Hi @hrj, in my understanding Scallop is adding "--help", not "-h". When -h is given as a program argument to CLIApp without having my change in place, following text is printed to the console:

-c, --config
--filter name=value [ name=value]... Transaction filters
-i, --input ...
-q, --quiet
-r, --report ...
-X, --unversioned
--help Show help message
--version
CLI Argument error: Unknown option 'h'

With my change you see:

-c, --config
--filter name=value [ name=value]... Transaction filters
-i, --input ...
-q, --quiet
-r, --report ...
-X, --unversioned
-h, --help
--version

I'll check the tests...

@hrj
Copy link
Owner

hrj commented Jan 27, 2017

@gerdreiss Ah yes, you are right about -h.

About the test failure, FYI, I reported it upstream: scallop/scallop#129

@hrj
Copy link
Owner

hrj commented Jan 27, 2017

@gerdreiss Rogach replied on the scallop issue, with multiple ideas. I like the idea of fixing the onError() function so that it handles the Help and Version exceptions. The behaviour would be very much like the super behavior, except we won't call sys.exit(). Instead we would rethrow the exception, as we do for ScallopException.

Do you want to take a stab at it?

@gerdreiss
Copy link
Contributor Author

@hrj Sounds good, I'll give it a try

@coveralls
Copy link

Coverage Status

Coverage increased (+0.5%) to 70.249% when pulling 23b18d9 on gerdreiss:help-option into a232d93 on hrj:master.

@hrj hrj merged commit ad21147 into hrj:master Jan 27, 2017
@gerdreiss gerdreiss deleted the help-option branch January 27, 2017 12:06
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.

None yet

3 participants