Skip to content

Hack around Python 2 ASCII encoding bug / incompatibility #8

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

Merged
merged 6 commits into from
Sep 20, 2017

Conversation

PiDelport
Copy link
Owner

For context, see previous PRs #4 and #7.

This expands the test coverage to run against ASCII, adds a workaround for PythonCharmers/python-future#256 (which testing against ASCII runs into), and enables the hack for Python 2 ASCII (see comments).

@PiDelport PiDelport added the bug label Sep 19, 2017
@PiDelport PiDelport added this to the Release 0.2 milestone Sep 19, 2017
@PiDelport PiDelport self-assigned this Sep 19, 2017
@codecov
Copy link

codecov bot commented Sep 19, 2017

Codecov Report

Merging #8 into master will decrease coverage by 69.63%.
The diff coverage is 38.09%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master       #8       +/-   ##
===========================================
- Coverage   95.83%   26.19%   -69.64%     
===========================================
  Files           5        5               
  Lines         168      668      +500     
  Branches       26      112       +86     
===========================================
+ Hits          161      175       +14     
- Misses          4      488      +484     
- Partials        3        5        +2
Impacted Files Coverage Δ
tests/test_os.py 100% <ø> (+14.28%) ⬆️
tests/test_extra.py 100% <100%> (ø) ⬆️
src/backports/os.py 14.13% <7.14%> (-79.46%) ⬇️
tests/test_helpers.py 95.83% <0%> (-4.17%) ⬇️

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 848c092...f69d58f. Read the comment docs.

@pombredanne
Copy link

Excellent! I wonder if vstinner/misc@a5f90a0#diff-b500f48c9778753dc97ebc453352f351R80 was not also about this....
FWIW I also stumbled on this other implementation https://github.com/chromium/caterpillar/blob/master/src/surrogateescape.py but I am not sure it works entirely the same.

I confirm that this now works correctly work with your fix using a degenerated locale!
Thanks ++
@sschuberth FYI.

@PiDelport
Copy link
Owner Author

PiDelport commented Sep 20, 2017

Hmm, I'm not sure what's up with the test coverage reporting. Something seems to be going wrong with the collection. :/

Running a coverage report locally gives the expected coverage, so I'm going to assume something with the CI integration is wonky; I'll go ahead and merge this, and worry about the CI reporting later.

@PiDelport PiDelport merged commit 9aec8f8 into master Sep 20, 2017
@pombredanne
Copy link

FWIW the coverage issue may be about how codecov merges reports from multiple runs in the CI matrix...

@pombredanne
Copy link

And you even pushed this to Pypi... https://pypi.python.org/pypi/backports.os/0.1.1 Thank you very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants