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.

Sign up for GitHub

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

Jump to bottom

Fix code for Python3 #11

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Fix code for Python3 #11

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
e494b25
Bump test step timeout in GHA to 3min
webknjaz Mar 16, 2021
8d040f8
Fix code for Python3
szymonlipinski Dec 17, 2020
5e603e5
Add six to required packages.
szymonlipinski Dec 18, 2020
c58b614
A better fix for python3 support
szymonlipinski Dec 18, 2020
c3eb18f
Revert changes for the StreamLogger and FileLogger
szymonlipinski Jan 28, 2021
86f754c
Fix encoding name
szymonlipinski Feb 3, 2021
79d20a6
Revert last changes to check tests
szymonlipinski Feb 3, 2021
1bb97e3
Revert "Revert last changes to check tests"
szymonlipinski Feb 3, 2021
0bf9c28
Fix loggers code
szymonlipinski Feb 3, 2021
35017cd
Fix loggers code
szymonlipinski Feb 3, 2021
7876e3a
Revert the last loggers changes
szymonlipinski Feb 4, 2021
ed05583
clean code
szymonlipinski Feb 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .github/workflows/python-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,17 +187,17 @@ jobs:
-vv
# macOS-latest + CPython completes in 9-15s
# macOS-11.0 + CPython completes in 8s-12s
# ubuntu-16.04 + CPython completes in 8s-13s
# ubuntu-16.04 + CPython completes in 8s-2m12s
# ubuntu-16.04 + PyPy completes in 13s-16s
# ubuntu-18.04 + CPython completes in 8s-13s
# ubuntu-18.04 + CPython completes in 8s-2m12s
# ubuntu-18.04 + PyPy completes in 13s-15s
# ubuntu-20.04 + CPython completes in 8s-14s
# ubuntu-20.04 + PyPy completes in 13s-16s
# windows-latest + CPython completes in 7s-9s
# windows-latest + PyPy completes in 13s-24s
# windows-2016 + CPython completes in 7s-11s
# windows-2016 + PyPy completes in 13s-20s
timeout-minutes: 2
timeout-minutes: 3
- name: Dump test logs on failure
if: failure()
shell: bash
Expand Down
7 changes: 5 additions & 2 deletions magicbus/plugins/loggers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,20 +30,23 @@ def log(self, msg, level):
if self.encoding is not None:
if isinstance(complete_msg, unicodestr):
complete_msg = complete_msg.encode(self.encoding)
else:
if not isinstance(complete_msg, unicodestr):
complete_msg = complete_msg.decode("utf-8")
Copy link
Contributor

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.

Copy link
Contributor

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")

Copy link
Contributor

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

Copy link
Author

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.


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):
Copy link
Member

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.

Copy link
Contributor

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

Copy link
Member

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.

Copy link
Contributor

@amol- amol- Feb 11, 2021 •
edited
Loading

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

Copy link
Member

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.

StreamLogger.__init__(self, bus, sys.stdout, level, format, encoding)


class StderrLogger(StreamLogger):

def __init__(self, bus, level=None, format=None, encoding='utf-8'):
def __init__(self, bus, level=None, format=None, encoding=None):
StreamLogger.__init__(self, bus, sys.stderr, level, format, encoding)


Expand Down