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 unicode decode error in result output #141

Merged

Conversation

jpellerin
Copy link
Member

Possible allternate fix for #116 (no tests yet, not ready to merge).

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.14%) when pulling daed1d3 on jpellerin:fix-unicode-decode-error-in-result into 36d026e on nose-devs:master.

@thedrow
Copy link
Member

thedrow commented Dec 11, 2013

This pull request does not encode according to the stream's encoding. Why should it supersede #130?

@jpellerin
Copy link
Member Author

I'm not saying it should, just that it's another possible approach. One
advantage it has over #130 (as currently written) is that #130 will still
crash if it is given unicode that can't be encoded according to the
stream's encoding, while version will not. Maybe combine the two
approaches, extend util.safe_decode to take an optional encoding param and
use that where #130 does?

On Wed, Dec 11, 2013 at 3:41 AM, Omer Katz notifications@github.com wrote:

This pull request does not encode according to the stream's encoding. Why
should it supersede #130 #130?


Reply to this email directly or view it on GitHubhttps://github.com//pull/141#issuecomment-30302992
.

@thedrow
Copy link
Member

thedrow commented Dec 11, 2013

Yup. That sounds right. Oh btw, I do think we need a regression test for this one but I'm not sure how to write one.

p.s.
Check the mailing list.

@jpellerin
Copy link
Member Author

After some further testing and work, this PR fixes the non-ascii output bug I'm seeing, but #130 doesn't. The bug has become super annoying, so I'm going to merge this now, and we can consider also merging #130 in the future, as I think it solves a different unicode/ascii issue.

jpellerin added a commit that referenced this pull request Feb 12, 2014
…sult

Fix unicode decode error in result output
@jpellerin jpellerin merged commit d7208b9 into nose-devs:master Feb 12, 2014
@thedrow
Copy link
Member

thedrow commented Feb 12, 2014

Great :) Thanks.

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.

None yet

3 participants