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

Force event to str in ConsoleRenderer #221

Merged
merged 5 commits into from Oct 16, 2019

Conversation

therefromhere
Copy link
Contributor

@therefromhere therefromhere commented Aug 2, 2019

Resolves #186

The only thing I'm not sure about is how this should behave with unicode in py2.7?

@hynek
Copy link
Owner

@hynek hynek commented Aug 2, 2019

Hm yeah this is a regression. It should be pulled out and protected using an if not isinstance(event, (six.text_type, bytes)) I think.

@hynek
Copy link
Owner

@hynek hynek commented Aug 15, 2019

Ping?

@therefromhere
Copy link
Contributor Author

@therefromhere therefromhere commented Aug 15, 2019

Sorry, I haven't had a chance to work on this.

@therefromhere
Copy link
Contributor Author

@therefromhere therefromhere commented Aug 15, 2019

I'd be happy to close this for now and reopen once py2 support has been dropped?

@hynek
Copy link
Owner

@hynek hynek commented Aug 16, 2019

Python 2 hasn’t been dropped for structlog and I don’t plan on it anytime soon because I need it myself in the foreseeable future. 😇 We’ll only be removing 3.4 from CI in the next release.

@neumachen
Copy link

@neumachen neumachen commented Oct 10, 2019

@hynek will this get merged anytime soon? I'm finding myself the need to log in a dict but be renderable as as string in development for pretty output.

@therefromhere
Copy link
Contributor Author

@therefromhere therefromhere commented Oct 10, 2019

@magicalbanana not speaking for @hynek , but unlikely, since it doesn't support py2. I'd be happy to close since I probably won't have time to make it robustly py2 compatible soon, unless someone else wants to pick this up?

@hynek
Copy link
Owner

@hynek hynek commented Oct 14, 2019

I think this might be much easier than thought since structlog is depending on six. I suspect it might be enough to replace str by six.text_type?

@therefromhere
Copy link
Contributor Author

@therefromhere therefromhere commented Oct 14, 2019

I think this might be much easier than thought since structlog is depending on six. I suspect it might be enough to replace str by six.text_type?

Possibly? Though I think that would subtly change the behavior on py2, because that would force event etc to be unicode strings, whereas currently they'll be byte strings (assuming that's what event_dict["event"] is).

@hynek
Copy link
Owner

@hynek hynek commented Oct 15, 2019

I keep forgetting how to push to other people's branches, so here's my suggested solution:

diff --git src/structlog/dev.py src/structlog/dev.py
index 42bb10a..372f7fc 100644
--- src/structlog/dev.py
+++ src/structlog/dev.py
@@ -8,7 +8,7 @@ Helpers that make development with ``structlog`` more pleasant.
 
 from __future__ import absolute_import, division, print_function
 
-from six import StringIO
+from six import PY2, StringIO, string_types
 
 
 try:
@@ -214,7 +214,10 @@ class ConsoleRenderer(object):
             )
 
         # force event to str for compatibility with standard library
-        event = str(event_dict.pop("event"))
+        event = event_dict.pop("event")
+        if not PY2 or not isinstance(event, string_types):
+            event = str(event)
+
         if event_dict:
             event = _pad(event, self._pad_event) + self._styles.reset + " "
         else:
diff --git tests/test_dev.py tests/test_dev.py
index 8da5a15..5e0b0c2 100644
--- tests/test_dev.py
+++ tests/test_dev.py
@@ -91,6 +91,17 @@ class TestConsoleRenderer(object):
 
         assert unpadded == rv
 
+    @pytest.mark.skipif(not six.PY2, reason="Problem only exists on Python 2.")
+    @pytest.mark.parametrize("s", [u"\xc3\xa4".encode("utf-8"), u"ä", "ä"])
+    def test_event_py2_only_stringify_non_strings(self, cr, s, styles):
+        """
+        If event is a string type already, leave it be on Python 2. Running
+        str() on unicode strings with non-ascii characters raises an error.
+        """
+        rv = cr(None, None, {"event": s})
+
+        assert styles.bright + s + styles.reset == rv
+
     def test_level(self, cr, styles, padded):
         """
         Levels are rendered aligned, in square brackets, and color coded.

Does that make sense?

I'm honestly a bit surprised that StringIO makes this work but I'll take it I guess…

@therefromhere
Copy link
Contributor Author

@therefromhere therefromhere commented Oct 15, 2019

Thanks, yeah that seems reasonable. I've added your patch as is.

hynek
hynek approved these changes Oct 16, 2019
@hynek hynek merged commit 46b8b64 into hynek:master Oct 16, 2019
22 checks passed
@hynek
Copy link
Owner

@hynek hynek commented Oct 16, 2019

Thanks!

@therefromhere therefromhere deleted the force_event_to_str branch Oct 16, 2019
hynek added a commit that referenced this issue Oct 16, 2019
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.

3 participants