Skip to content

Conversation

@frol
Copy link

@frol frol commented Sep 15, 2015

sys.stdout has encoding attribute.

This PR will be useful for my next PR to Invoke.

`sys.stdout` has `encoding` attribute
@bitprophet
Copy link
Owner

Thanks :) couple quick thoughts w/o actually testing anything yet:

  • Do we need to change getvalue() as well? It was the other spot that hardcoded 'utf-8' encoding stuff.
  • Will this break anything that used to rely on the implicit utf-8 encoding? Feels like the naïve case would end up being encoded as latin-1 now instead of utf-8. (This is hopefully ignorant paranoia on my part, but presumably I chose utf-8 earlier for a reason, instead of latin-1.)

@frol
Copy link
Author

frol commented Sep 16, 2015

I have just fixed getvalue.

I guess it shouldn't break anything if cc'ing streams have encoding attribute.

@frol
Copy link
Author

frol commented Sep 17, 2015

@bitprophet Is there anything to improve in this PR?

@bitprophet
Copy link
Owner

Nope, thanks. I just have too many projects (with too many users each :D) and too little time. Not everything's gonna be quick :(

Assuming this is required for one of the encoding tests in Invoke to work correctly, I'll doubtless merge this as a dependency of the other, whenever it's gotten to.

@frol
Copy link
Author

frol commented Sep 19, 2015

@bitprophet It used to be needed in my Invoke patch, but then I changed the logic around encoding, so it is not required at the moment. Nevertheless, it is good improvement anyway.

@bitprophet
Copy link
Owner

Gotcha, good to know, thanks (& I agree, more encoding control is usually better :))

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.

2 participants