Python3 syslog fix #1283
Python3 syslog fix #1283
Conversation
…t when running under python3
|
This pull request addresses issue #1282 |
|
It appears the |
|
I'm not sold that this is the correct way to address this problem, but I thought I'd go ahead and show a potential solution. One thing of note, because the |
Joppie
force-pushed
the
python3_syslog_fix
branch
from
08f031f to
7f6a133
Compare
|
cc: @vsajip |
|
Actually, the issue addressed appears ro be #1282 |
vsajip
left a comment
vsajip
left a comment
There was a problem hiding this comment.
There should be another test that verifies that whether you create a LogRecord with a text msg or with a bytes msg, the message key in the resulting value of asdict() always contains text, not bytes.
It's also worth testing what happens if the as_string() is applied in the LogRecord constructor - messages should always be text, even on Python 2!
| msg = self.msg % self.kw | ||
| msg = as_string(self.msg) % self.kw | ||
| else: | ||
| msg = self.msg |
There was a problem hiding this comment.
Why not as_string(self.msg) here, too? asdict() should be returning text here rather than bytes for the message key, whether or not there are arguments.
| message = params['message'] | ||
| for line in message.rstrip('\n').split('\n'): | ||
| params['message'] = line | ||
| split_char = b'\n' if isinstance(message, bytes) else '\n' |
There was a problem hiding this comment.
Since in the return value from asdict() the message should always be text, this shouldn't ne needed, right?
| def test_emit_ascii_noerror(self): | ||
| handler = self._makeOne() | ||
| record = self._makeLogRecord('hello!') | ||
| record = self._makeLogRecord(b'hello!') |
There was a problem hiding this comment.
Shouldn't the test verify both text and bytes inputs here, rather than just bytes?
|
See also #1284 for the alternative approach I suggested. |
|
Generally, I believe messages, formats etc. should logically be Unicode ( |
|
Just to confirm - did you try the #1284 fix in your environment? Did messages show up in syslog with that fix applied? |
|
I just tested it and it does fix the issue in the environment I am using. Thanks. |
3 participants
No description provided.