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.

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

Fix crash caused by mixup of bytes and str #23

Merged
merged 2 commits into from
Jun 13, 2017
Merged

Conversation

dvdkon
Copy link
Contributor

@dvdkon dvdkon commented May 24, 2017

This fixes dependency-based checking for "outdatedness".

Popen.communicate() return a tuple of bytes, but the rest of the code
expects str. Decoding the bytes object fixes this. Some (very weird)
outputs may now fail with UnicodeDecodeError, but that'd probably be
indicative of a bug elsewhere.

Popen.communicate() return a tuple of bytes, but the rest of the code
expects str. Decoding the bytes object fixes this. Some (very weird)
outputs may now fail with UnicodeDecodeError, but that'd probably be
indicative of a bug elsewhere.
@natevw
Copy link
Collaborator

natevw commented Jun 2, 2017

Ah, did Python 3 change the return value? Does your patch still work on Python 2?

@dvdkon
Copy link
Contributor Author

dvdkon commented Jun 2, 2017

Yes. This should still work on Python 2, as (at least in my version) it has a method "decode" on both "normal" and Unicode strings. However, the rest of the code might not work anymore, as it uses print as a function without importing the necessary compatibility modules (that's only a problem with verbose mode, though).

@natevw
Copy link
Collaborator

natevw commented Jun 5, 2017

Great, thanks. Yeah I noticed the print thing got introduced in 1492c85, looks like we need a from __future__ import print_function at the top of the file. It probably wouldn't hurt to add the print compatibility to this pull request, if you don't mind?

IIRC, I would be able to merge this pull request here on github but can't publish any updates on pip. So I'll let @j0hnsmith hopefully take care of this when he gets a chance.

@j0hnsmith
Copy link
Owner

@natevw Cool, just give me a nudge when it's ready to go to pypi.

@dvdkon
Copy link
Contributor Author

dvdkon commented Jun 10, 2017

Done.

@natevw natevw merged commit c8afeaa into j0hnsmith:master Jun 13, 2017
@natevw
Copy link
Collaborator

natevw commented Jun 13, 2017

Thanks, I've merged this. @j0hnsmith can you publish on pypi?

@natevw natevw mentioned this pull request Oct 17, 2017
@karimone
Copy link

The version must be updated and the package is not on pypi.

@j0hnsmith
Copy link
Owner

Updated the version in setup.py and pushed a 0.6.2 tag to github. I'm having trouble pushing a new version to pypi, I'll do it asap.

@j0hnsmith
Copy link
Owner

0.6.2 is now on pypi

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

Successfully merging this pull request may close these issues.

4 participants