-
-
Notifications
You must be signed in to change notification settings - Fork 7
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 code for Python3 #11
base: main
Are you sure you want to change the base?
Conversation
Could you explain what it is that you're trying to fix? All I see is some style and default args changes. |
Sure. I get lots of |
@szymonlipinski sounds like you pass the wrong data types when calling magicbus APIs. |
@webknjaz I think that the issue is in
The issue can be replicated with a very short snippet:
I think that the PR is wrong as it changes too many things (it changes also |
Fair enough |
It looks like the tests hang on python 2.7 even if I reverted all changes from this PR: https://travis-ci.org/github/cherrypy/magicbus/jobs/757368941 |
|
||
else: | ||
if not isinstance(complete_msg, unicodestr): | ||
complete_msg = complete_msg.decode("utf-8") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if in this case we might want to add errors="backslashreplace"
given that we are mostly guessing utf-8
might be a good choice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PPS: This can probably just become
if isinstance(complete_msg, unicodestr):
encoding = self.encoding or "utf-8"
complete_msg = complete_msg.encode(encoding, errors="backslashreplace")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I misread the two if
branches o_O
You are encoding
in one and decoding
in the other, my suggestion of squashing the two branches was totally wrong :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, you're right. I haven't noticed that too :) I have reverted the changes. I think it's fine now.
magicbus/plugins/loggers.py
Outdated
complete_msg = complete_msg.decode("utf-8") | ||
if isinstance(complete_msg, unicodestr): | ||
encoding = self.encoding or "utf-8" | ||
complete_msg = complete_msg.encode(encoding, errors="backslashreplace") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, I meant, both ifs can be removed :D
if self.encoding is not None:
if isinstance(complete_msg, unicodestr):
complete_msg = complete_msg.encode(self.encoding)
else:
if isinstance(complete_msg, unicodestr):
encoding = self.encoding or "utf-8"
complete_msg = complete_msg.encode(encoding, errors="backslashreplace")
VS
if isinstance(complete_msg, unicodestr):
encoding = self.encoding or "utf-8"
complete_msg = complete_msg.encode(encoding, errors="backslashreplace")
@webknjaz Hey, could you take a look at the failing test? It failed also when I reverted all the changes made by this PR. It looks like there is some bigger problem somewhere. All the tests pass on my computer for python2 and python3. |
Since it is only happening in CIs and hang on the tests that have TTY in their name, my best guess is that the CIs don't allocate a TTY when running these tests and that's what's causing the problem. I wonder if we could allocate a TTY on our own in such envs... |
|
||
self.stream.write(complete_msg) | ||
self.stream.flush() | ||
|
||
|
||
class StdoutLogger(StreamLogger): | ||
|
||
def __init__(self, bus, level=None, format=None, encoding='utf-8'): | ||
def __init__(self, bus, level=None, format=None, encoding=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how this is related to the problem in question. Could you drop changing the defaults too? It is a behavior change at best (and a public interface change at worst. This does not seem to be fixing bugs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the default change is actually the fix, otherwise you would have to explicitly pass encoding=None
everywhere to make sure this works at all on PY3, as there is no way you can have StdoutLogger
work on PY3 if it's encoding.
See
>>> StdoutLogger(mock.Mock(id=5), encoding="utf8").log("HI", level=20)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/Users/amol/wrk/crunch/venv/lib/python3.7/site-packages/magicbus/plugins/loggers.py", line 34, in log
self.stream.write(complete_msg)
TypeError: write() argument must be str, not bytes
VS
>>> StdoutLogger(mock.Mock(id=5), encoding=None).log("HI", level=20)
[b'2021-02-11T15:51:15.308588'] (Bus 5) HI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a bytes vs unicode py2/3 issue. It's usually better to always have this normalized internally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't personally consider this a string encoding/decoding issue.
The problem is related to the fact that sys.stdout
and sys.stderr
are text mode files, so you can't send bytes to them at all in PY3. The whole "encoding" option doesn't make much sense for sys.stdout/err
in my opinion.
If you want to change the encoding of them you are meant to do sys.stdout.reconfigure(encoding='utf-8')
In PY2 sys.stdout
/sys.stderr
like most io, were mostly agnostic to what you throw at them and thus tried to do their best with whatever they received, but you were still in theory meant to send unicode to them and get it printed according to sys.stdout.encoding
like on PY3.
I think that it was mostly an hack the fact that magicbus allowed you to send bytes to sys.stdout
at all totally ignoring what was sys.stdout.encoding
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. Honestly, it'd be great to integrate stdlib logging
across all CherryPy projects instead of reinventing the wheel.
This all looks like a lack of proper text compat functions. See some examples:
A common technique is to either convert most of the stuff to "native" strings or unicode/text with exceptions for passing them as args to certain stdlib interfaces (that require "native" most of the time). In general, it's a good idea to always accept text and "native" from external calls because with bytes we cannot (shouldn't) automatically assume the encoding. |
This is reproducible (gets stuck) for me locally with
But it passes if I tell pytest not to capture stdin/stdout:
(same with |
Looking further, I now remember that it's #7 that I attempted to fix with #8. When I add
|
@szymonlipinski so I hacked that those annoying unrelated failing tests and it should be fine now. Although, I've added an excessive GHA matrix that is flaky at times. Could you rebase this PR so we could see how it goes? |
This reverts commit 28e9064.
368f806
to
ed05583
Compare
No description provided.