fix(python): handle source decoding errors #1164
Conversation
As described in nedbat#1160, occasionally some shared object paths make their way into the coverage report. When they get to `get_python_source` the call to `source_encoding` fails because the content of these files is binary. The change allows `get_python_source` to handle these cases gracefully while still reporting the issue. Fixes nedbat#1160.
| source = source.decode(source_encoding(source), "replace") | ||
| try: | ||
| source = source.decode(source_encoding(source), "replace") | ||
| except Exception as e: |
There was a problem hiding this comment.
I would much rather catch a more specific error, in a more specific place. The exception is happening inside source_encoding, so why not put the except-clause in that function?
There was a problem hiding this comment.
This felt like a good place for catching generic decoding errors and not just the one in the linked issue. Of course, I could add a specific exception handling in source_encoding if that's preferable.
There was a problem hiding this comment.
The reason this is a bad place for the except is that s.decode(..., "replace") can't raise an exception. In that line of code, it's only the source_encoding() call that can fail.
There was a problem hiding this comment.
Oh but this could be fixed by just moving source_encoding on its own line and wrapping it with try...except 😜 . Anyway, I'll have a closer look as soon as I can.
There was a problem hiding this comment.
Right, I've had another look and the idea of adding checks to _source_encoding_py2 is not very appealing. Not to mention that they would have to be duplicated in _source_encoding_py3.
There was a problem hiding this comment.
Well, _source_encoding_py2 is gone on master, we've dropped Python 2 support.
There was a problem hiding this comment.
Ah I've just realised that support for Python 3.5 is also being dropped, which is not ideal for our usage. This prompted me to test on Python 3.7 with the master branch of coveragepy and the problem does not present itself 🙁 .
|
This has now been fixed with #1252 |
As described in #1160, occasionally some shared object paths make their way into the coverage report. When they get to
get_python_sourcethe call tosource_encodingfails because the content of these files is binary. The change allowsget_python_sourceto handle these cases gracefully while still reporting the issue.Fixes #1160.