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

Move from optparse to argparse #237

Merged
merged 5 commits into from Mar 7, 2018

Conversation

Projects
None yet
2 participants
@latk
Member

latk commented Mar 6, 2018

Argparse is the better optparse. The only reason why we hadn't already switched was that argparse was only introduced in Python 2.7. Luckily there's a backport that can be installed from PyPI, so I added a version-dependent dependency.

The difficult part was enabling the necessary tests on Travis CI first. This surfaced a few compatibility problems that were accumulated in the recent refactorings. Especially subprocess and runpy lack many convenience functions, and format strings "foo{bar}" must not have empty placeholders {}. The tests cannot rely on the python -m gcovr style invocation, but the gcovr entrypoint script is available anyway. Now all tests except for an XML example pass.

latk added some commits Mar 6, 2018

fix format strings for Python 2.6
In a format string, placeholders "foo{}bar" must be non-empty.
fix tests for Python 2.6
The "python -m module" style to execute modules was first introduced in
Python 2.7.
move from optparse to argparse
Argparse has
 - a better ecosystem
 - a more convenient API
 - actual support for positional arguments (our search_paths)
@codecov

This comment has been minimized.

codecov bot commented Mar 6, 2018

Codecov Report

Merging #237 into master will decrease coverage by 0.02%.
The diff coverage is 92.72%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #237      +/-   ##
==========================================
- Coverage   87.37%   87.35%   -0.03%     
==========================================
  Files          12       12              
  Lines        1339     1336       -3     
  Branches      243      243              
==========================================
- Hits         1170     1167       -3     
  Misses        115      115              
  Partials       54       54
Impacted Files Coverage Δ
gcovr/__main__.py 88.05% <100%> (-0.27%) ⬇️
gcovr/coverage.py 98.57% <100%> (ø) ⬆️
gcovr/tests/test_args.py 100% <100%> (ø) ⬆️
gcovr/tests/test_gcovr.py 97.56% <100%> (ø) ⬆️
gcovr/gcov.py 81.96% <42.85%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dfb74bd...289a478. Read the comment docs.

@latk latk referenced this pull request Mar 7, 2018

Merged

Parallelize main loop #239

@mayeut

mayeut approved these changes Mar 7, 2018

👍

@mayeut

This comment has been minimized.

Contributor

mayeut commented Mar 7, 2018

@latk, any clue why the one test fails ?

@latk

This comment has been minimized.

Member

latk commented Mar 7, 2018

The remaining test failure in the example XML just seems to be a whitespace issue:

E           ValueError: (<root>.coverage.sources.source) Values differ:
E           baseline:
E           u'.'
E           output:
E           u'   .'

possibly because of the --xml-pretty layout. There may be additional differences later in the file. The element in the error message is just the report directory.

It would be good to perhaps investigate this failure at some point, but the other tests pass without a problem so I don't think this failure is very important.

@latk latk merged commit f997eda into gcovr:master Mar 7, 2018

4 checks passed

codecov/patch 92.72% of diff hit (target 87.37%)
Details
codecov/project Absolute coverage decreased by -0.02% but relative coverage increased by +5.34% compared to dfb74bd
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@latk latk deleted the latk:argparse branch Mar 7, 2018

@latk latk removed the needs review label Mar 7, 2018

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