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

3585 - Fix broken arguments including -h flag #3587

Merged
merged 3 commits into from Jun 22, 2018

Conversation

Projects
None yet
3 participants
@hamptus
Contributor

hamptus commented Jun 20, 2018

This is a fix for issue #3585

The bug was introduced when DEBUG_ALL_CHOICE was placed inside a new list here: bbede2e#diff-98596315ae5d573e2ec661f6cd25f560R181

argparse was having trouble parsing the arguments.

@hamptus hamptus referenced this pull request Jun 20, 2018

Closed

pyinstaller -h is broken #3585

@bjones1

This comment has been minimized.

Member

bjones1 commented Jun 21, 2018

Thanks for correcting my silly mistake!

@htgoebel htgoebel self-requested a review Jun 21, 2018

@htgoebel

This comment has been minimized.

Member

htgoebel commented Jun 21, 2018

@bjones1 Prior to merging, please verify --debug actually works as expected:

  • help should work - there is also an issue: _split_lines() must be called with RawTextHelpFormatter instance as first argument
  • using the options works as expected: I tried pyinstaller --debug foo and got: "argument -d/--debug: invalid choice: 'foo'"
  • Also please mind the commit message(s) to follo the guide-line.
  • What about adding a simple pyinstaller -h to the tests at travis (first stage)?

Thanks.

@htgoebel

Please verify this actually solves all issues.

@bjones1

This comment has been minimized.

Member

bjones1 commented Jun 21, 2018

@htgoebel: oops, I'll be more careful in the future. Another project is looming, so I'm hurrying to wrap up loose ends before I move to that.

I'll update the usage of _split_lines and include running pyinstaller -h in the Travis and Appveyor scripts.

However, the error produced by pyinstaller --debug foo is correct -- debug is looking for another option. pyinstaller --debug -y foo is fine, because argparse understands that the -y is a separate option. pyinstaller --debug all foo also works. Unfortunately, this makes the --debug option a bit confusing to use. Ideas? I could require a parameter (so that --debug alone is an error), but there's no way I know of to make argparse work with pyinstaller --debug foo. (Note that argparse parses --debug all and --debug=all identically -- there's no way I know of around this.)

@bjones1

This comment has been minimized.

Member

bjones1 commented Jun 21, 2018

@hamptus, would you titled your commit building: fix broken choices parameter for the --debug command-line argument?

@hamptus

This comment has been minimized.

Contributor

hamptus commented Jun 22, 2018

@bjones1 Do you want me to change the commit message or the PR title?

@bjones1

This comment has been minimized.

Member

bjones1 commented Jun 22, 2018

The commit message.

@hamptus hamptus force-pushed the hamptus:3585_fix_broken_command_flags branch from 4e4ed99 to 4a107cd Jun 22, 2018

@hamptus

This comment has been minimized.

Contributor

hamptus commented Jun 22, 2018

@bjones1 Done!

@bjones1

This comment has been minimized.

Member

bjones1 commented Jun 22, 2018

Thanks!

@bjones1

This comment has been minimized.

Member

bjones1 commented Jun 22, 2018

@htgoebel, I'll go ahead and merge, since it addresses all your points above except for the UI problem (pyinstaller --debug foo fails, since argparse treats foo as an optional argument to --debug). Perhaps we just specify that --debug should be placed last on the command line? That is, pyinstaller foo --debug works.

@bjones1 bjones1 merged commit c06d853 into pyinstaller:develop Jun 22, 2018

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
@htgoebel

This comment has been minimized.

Member

htgoebel commented Jun 26, 2018

Another project is looming, so I'm hurrying to wrap up loose ends before I move to that.

"In der Ruhe liegt die Kraft" - German proverb :-) Never mind, we are all volunteers. Thanks for taking care of the fix.

Regarding the UI problem: I'd keep it as it is. We just need to keep it in mind for examples. Another way should be (untested) pyinstaller --debug -- foo, with -- marking the end of the optional arguments.

bjones1 added a commit that referenced this pull request Jun 27, 2018

@bjones1

This comment has been minimized.

Member

bjones1 commented Jun 27, 2018

Sounds good, done in 84c1aaf. Your suggested alternative does seem work, though I used debug=all as (hopefully) less confusing in the docs.

@htgoebel htgoebel added this to the PyInstaller 3.4 milestone Sep 2, 2018

cowo78 pushed a commit to cowo78/pyinstaller that referenced this pull request Dec 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment