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.

Sign up for GitHub

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

Jump to bottom

Change optparse for argparse. #238

Merged
merged 24 commits into from May 19, 2017
Merged

Change optparse for argparse. #238

merged 24 commits into from May 19, 2017

Conversation

froi
Copy link
Contributor

@froi froi commented Feb 26, 2017

  • Change optparse for the more modern argparse.
  • Refactored main() to fit new argparse code.
  • Added two subcommands: encode and decode
  • Added default functions for encode and decode subcommands
  • Replaced implicit arguments with standard Python syntax in usage text.

Change optparse for argparse.
f0b903f 
- Change optparse (deprecated) for the more modern argparse.
- Added two subcommands: encode and decode
- Added default functions for encode and decode subcommands
- Replaced implicit arguments with standard Python syntax in usage text.
-Refactored main() to fit new argparse code.
@coveralls
Copy link

coveralls commented Feb 26, 2017 •
edited

Coverage Status

Coverage remained the same at 100.0% when pulling f0b903f on froi:change-optparse-to-argparse into 0a4b8dd on jpadilla:master.

Fix imports order.
de42a87 
- Moved argparse import to first import.
@coveralls
Copy link

coveralls commented Feb 26, 2017 •
edited

Coverage Status

Coverage remained the same at 100.0% when pulling de42a87 on froi:change-optparse-to-argparse into 0a4b8dd on jpadilla:master.

Fix flake8 issues.
041d689 
- Fixed flake8 spacing issues.
@coveralls
Copy link

coveralls commented Feb 26, 2017 •
edited

Coverage Status

Coverage decreased (-1.6%) to 98.379% when pulling 041d689 on froi:change-optparse-to-argparse into 0a4b8dd on jpadilla:master.

@jpadilla
Copy link
Owner

@froi looks good to me, thanks! Coverage decreased because encode_payload() and decode_payload() add API surface which we didn't have before. We currently don't have any tests for the CLI tool.

@froi
Copy link
Contributor Author

froi commented Feb 26, 2017 •
edited

Let me see if I can make that happen in a sane way. @jpadilla Do you want me to add that to this PR or another?

@jpadilla
Copy link
Owner

@froi here's fine.

Add test for cli
00ca237 
- Refactored __main__.py so it can be more testable.
- Created test_cli.py
- Added tests for encode_payload, decode_payload, build_argparse, and main functions.
- Added __main__,py to .coveragerc
@coveralls
Copy link

Coverage Status

Coverage decreased (-13.1%) to 86.9% when pulling 00ca237 on froi:change-optparse-to-argparse into 0a4b8dd on jpadilla:master.

Froilan Irizarry added 3 commits February 26, 2017 21:34
Fix linter errors.
0449e53
Fix tox errors:
a5a6202 
- Add py35 env to tox.ini
- Add flake8 excludes to tox.ini
- Change str.format() to str % str to be compatible with older Python envs.
- Removed pytest version constraint from setup.py
- Reordered envs in .travis.yml
- Added python 3.6 envs to .travis.yml
Revert test_require pytest to pytest==2.7.3
5675713
@coveralls
Copy link

Coverage Status

Coverage decreased (-9.6%) to 90.393% when pulling 5675713 on froi:change-optparse-to-argparse into 0a4b8dd on jpadilla:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-9.6%) to 90.393% when pulling 5675713 on froi:change-optparse-to-argparse into 0a4b8dd on jpadilla:master.

@coveralls
Copy link

coveralls commented May 16, 2017 •
edited

Coverage Status

Coverage decreased (-3.6%) to 96.361% when pulling f96a98a on froi:change-optparse-to-argparse into 0a4b8dd on jpadilla:master.

jpadilla
jpadilla requested changes May 16, 2017
View reviewed changes
.travis.yml Outdated
@@ -1,6 +1,6 @@
language: python
python:
- "3.5"
- "3.6"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is changing these required for this PR? There are some InterpreterNotFound errors in there, which might be fixed by #262.

tests/test_cli.py Show resolved Hide resolved
tests/test_cli.py Outdated Show resolved Hide resolved
Froilan Irizarry added 8 commits May 17, 2017 20:22
Change optparse for argparse.
79dbfad 
- Change optparse (deprecated) for the more modern argparse.
- Added two subcommands: encode and decode
- Added default functions for encode and decode subcommands
- Replaced implicit arguments with standard Python syntax in usage text.
-Refactored main() to fit new argparse code.
Fix imports order.
33fd8b8 
- Moved argparse import to first import.
Fix flake8 issues.
80fa761 
- Fixed flake8 spacing issues.
Add test for cli
f758fc0 
- Refactored __main__.py so it can be more testable.
- Created test_cli.py
- Added tests for encode_payload, decode_payload, build_argparse, and main functions.
- Added __main__,py to .coveragerc
Fix linter errors.
b6221ee
Fix tox errors:
096c800 
- Add py35 env to tox.ini
- Add flake8 excludes to tox.ini
- Change str.format() to str % str to be compatible with older Python envs.
- Removed pytest version constraint from setup.py
- Reordered envs in .travis.yml
- Added python 3.6 envs to .travis.yml
Fixed tests.
3f7cde4
Address PR comments.
f6612a7 
Added index to string format.
Removed unused import.
Fixed some flake8 warning/errors.
@coveralls
Copy link

coveralls commented May 18, 2017 •
edited

Coverage Status

Coverage decreased (-2.2%) to 97.817% when pulling f6612a7 on froi:change-optparse-to-argparse into 42ff8d4 on jpadilla:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-2.2%) to 97.817% when pulling f6612a7 on froi:change-optparse-to-argparse into 42ff8d4 on jpadilla:master.

jpadilla
jpadilla requested changes May 18, 2017
View reviewed changes
jwt/__main__.py Outdated
except Exception as e:
print('There was an unforseen error: ', e)
arg_parser.print_help()
sys.exit(1)


if __name__ == '__main__':
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@froi Noticed another thing missing.

  1. Run python setup.py install
  2. Run pyjwt

It'll fail with something like this:

$ pyjwt
Traceback (most recent call last):
  File "/Users/jpadilla/.pyenv/versions/pyjwt/bin/pyjwt", line 9, in <module>
    load_entry_point('PyJWT==1.5.0', 'console_scripts', 'pyjwt')()
TypeError: main() takes exactly 1 argument (0 given)

Since we're using the console_scripts entry point on our setup.py I think we should be able to drop just drop the if __name__ == '__main__': ... and have the main() method optionally set args to sys.argv[1:]. Let me know if this makes sense.

Copy link
Contributor Author

@froi froi May 18, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That'll work but it'll also make main() harder to test I think. I'll take a look at it.

@coveralls
Copy link

coveralls commented May 18, 2017 •
edited

Coverage Status

Coverage decreased (-1.2%) to 98.825% when pulling b614247 on froi:change-optparse-to-argparse into 42ff8d4 on jpadilla:master.

Froilan Irizarry added 3 commits May 18, 2017 01:34
Refactor and cleanup
238d9a8 
- Added test_encode_decode to up code coverage. decode_payload was previously not tested.

- Rename test_main. This was primarily to up coverage. sys.args are monkeypatched.
Remove commented import that is not in use
3878ff1
- Eliminated __name__ == '__main__"
a31bfe0 
- main now prints token to stdout
@coveralls
Copy link

coveralls commented May 18, 2017 •
edited

Coverage Status

Coverage decreased (-0.7%) to 99.264% when pulling a31bfe0 on froi:change-optparse-to-argparse into 42ff8d4 on jpadilla:master.

Froilan Irizarry added 2 commits May 18, 2017 22:59
Remove sys.exit.
e34f447 
- Removed sys.exit it was interferring with test.
Reaise code cov
4466782 
- Reordered imports
- Added test_decode_payload_raises_decoded_error_isatty
- Added test_main_throw_exception
- Monkeypatched sys.stdin.isatty and sys.stdin.read
- Monkeypatched argparse.ArgumentParser.parse_args to force exception in __main__.main
@coveralls
Copy link

coveralls commented May 19, 2017 •
edited

Coverage Status

Coverage remained the same at 100.0% when pulling 4466782 on froi:change-optparse-to-argparse into 42ff8d4 on jpadilla:master.

@coveralls
Copy link

coveralls commented May 19, 2017 •
edited

Pull Request Test Coverage Report for Build 148

  • 44 of 52 (84.62%) changed or added relevant lines in 1 file are covered.
  • 52 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-9.6%) to 90.393%
Changes Missing Coverage Covered Lines Changed/Added Lines %
jwt/main.py 44 52 84.62%
Files with Coverage Reduction New Missed Lines %
jwt/contrib/algorithms/pycrypto.py 25 3.85%
jwt/contrib/algorithms/py_ecdsa.py 27 6.9%
Totals Coverage Status
Change from base Build 139: -9.6%
Covered Lines: 621
Relevant Lines: 687
💛 - Coveralls

@coveralls
Copy link

coveralls commented May 19, 2017 •
edited

Coverage Status

Coverage remained the same at 100.0% when pulling ce4d420 on froi:change-optparse-to-argparse into 42ff8d4 on jpadilla:master.

4 similar comments
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling ce4d420 on froi:change-optparse-to-argparse into 42ff8d4 on jpadilla:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling ce4d420 on froi:change-optparse-to-argparse into 42ff8d4 on jpadilla:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling ce4d420 on froi:change-optparse-to-argparse into 42ff8d4 on jpadilla:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling ce4d420 on froi:change-optparse-to-argparse into 42ff8d4 on jpadilla:master.

@jpadilla jpadilla merged commit 328b3d8 into jpadilla:master May 19, 2017
@froi froi deleted the change-optparse-to-argparse branch May 19, 2017 03:32
@jpadilla
Copy link
Owner

@froi Thank you!

@vorpal-buildbot vorpal-buildbot mentioned this pull request Jun 15, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpadilla jpadilla jpadilla approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@froi @coveralls @jpadilla