Fix utf8 encoding on py2

[apiraino]

While working on my plugin, I noticed that logging utf-8 strings raises a UnicodeEncodeError at line 485.

I think the issue can be fixed by using in the module:

s = "Amthrÿa - Anesthesia Survival"
self.py3.log(s.decode('utf-8'))

and applying this patch to py3.py, which removes the need to cast string to utf-8 with u'hey hey'.

Unfortunately I couldn't easily find a way to write a unit-test for this patch (any help on that would be great!).

Opinions?

Thanks for reviewing!

[lasers]

Possible alternative.

diff --git a/py3status/py3.py b/py3status/py3.py
index fca9448..5e8c4fc 100644
--- a/py3status/py3.py
+++ b/py3status/py3.py
@@ -480,6 +480,10 @@ class Py3:
                 message = "\n" + message
         except:  # noqa e722
             pass
+        try:
+            message = message.decode()
+        except (AttributeError, UnicodeDecodeError):
+            pass
         message = "Module `{}`: {}".format(self._module.module_full_name, message)
         self._py3_wrapper.log(message, level)
 

EDIT: Maybe I'm confused. Error seems to be caused by adding decode() in the module?

[apiraino]

Hi @lasers thanks for having a look!

I did some tests and it looks to me that the decode('utf-8') in the plugin module is needed, but I think for this patch that is not relevant.

Using a try..except block make that more explicit indeed, but then one would put need to protect in that way every string coming from outside the python module, which I find a bit cumbersome. Importing
unicode_literals otoh should solve all utf-8 issues without touching the code and making the module future-proof.

py2 utf-8 strings have always giving me headaches ...

[tobes]

I did some tests and it looks to me that the decode('utf-8') in the plugin module is needed

The module shouldn't need to have to care. We should do the gatekeeping at some place.

@lasers we probably want to hit any UnicodeDecodeError because to me that suggests things are bad.

I'll try to look at the issue.

@apiraino thanks for highlighting the problem. However I'm not sure this PR actually changes anything.

from my point of view the error is s = "Amthrÿa - Anesthesia Survival" because that is not very python2 friendly as every I could be wrong here I've just skimmed things.

With the typo in mpris.py that should be done as a separate PR

[apiraino]

from my point of view the error is s = "Amthrÿa - Anesthesia Survival" because that is not very python2 friendly as every I could be wrong here I've just skimmed things.

Could be (not 100% sure me either). Anyway I was showing a testcase. I can reproduce the same issue logging a variable that contains a utf-8 string.

If I omit the utf8 decoding in the plugin module, then the error shows up again, so it might be the case to leave it anyways? If you want to manage this completely in the "core", it could be moved inside the logger facility (def log); I think it's safe and seems to work either:

--- a/py3status/py3.py
+++ b/py3status/py3.py
@@ -482,6 +482,7 @@ class Py3:
                 message = "\n" + message
         except:  # noqa e722
             pass
+        message = message.decode('utf-8')
         message = "Module `{}`: {}".format(self._module.module_full_name, message)
         self._py3_wrapper.log(message, level)

makes sense?

With the typo in mpris.py that should be done as a separate PR

agreed, I was just trying to figure out why the CI fails on me, while locally tests are fine 🤔 I already opened the pr so this is exposed to the upstream (in general I rebase the pr).

[tobes]

Anyway I was showing a testcase. I can reproduce the same issue logging a variable that contains a utf-8 string.

Some code that causes the issue is always nice to see :)

while locally tests are fine

Travis seems to like breaking things just to keep people busy

[apiraino]

Anyway I was showing a testcase. I can reproduce the same issue logging a variable that contains a utf-8 string.

Some code that causes the issue is always nice to see :)

👍

And a unittest even better (see my first comment). This code, for example, reproduces the issue.

[apiraino]

I think this should do. Feel free to refactor something else in another PR and close this one if not deemed suitable in some aspects 👍

[apiraino]

reverted the unrelated fix

[ultrabug]

Thanks for your proposal @apiraino !

Travis is not happy tho as you can see, could you have a look please?

https://travis-ci.org/ultrabug/py3status/jobs/460696072

[apiraino]

Thanks for your proposal @apiraino !

Travis is not happy tho as you can see, could you have a look please?

https://travis-ci.org/ultrabug/py3status/jobs/460696072

yes I did notice that a couple of days ago but I still haven't sorted that out. I can't reproduce that on my workstation :-/ (Running py2 tests with version 2.7.15+). Any suggestion is welcome!

EDIT: ok need to run tox -e py27 to reproduce

[lasers]

Try this:

diff --git a/py3status/modules/mpris.py b/py3status/modules/mpris.py
index e8429a4..609a576 100644
--- a/py3status/modules/mpris.py
+++ b/py3status/modules/mpris.py
@@ -596,7 +596,7 @@ class Py3status:
             _title = self._data.get('title', None)
             if _title is not None:
                 if self.debug:
-                    self.py3.log(u'[{}] title: "{}"'.format(self.audio_perc, _title))
+                    self.py3.log('[{}] title: "{}"'.format(self.audio_perc, _title))
                 self._maybe_save_audio()
                 _curr_vol, _ = self._get_volume()
                 if _title == '' or any(k in _title.lower() for k in self.state_quiet_keywords):

[apiraino]

sorry @lasers my comment was not clear, I was referring to the bit in my comment that Travis was not happy about my patch :^)

Well, now Travis, too, seems to be happy. Does this fix makes sense? In case you're happy with this, I will rebase this pr.

[lasers]

Does this fix makes sense?

That'll be up to @tobes.

Just a heads up. #1584 "[RFC] Get rid of Python 2.7".

1. Change to '' already work. Maybe we want to stay with that instead.
2. I'd wait for that RFC to be discussed first. Otherwise, we look into this PR and to start focusing on py3.py and others to support Python2.7 now only to start focusing on simplifying things to support Python3+ and eventually fixing things to support Python3.6+ for 4.0.

[ultrabug]

Python2.7 is indeed not the way to go

@apiraino how badly do you need this in really? TBH I'd be in favor to touch as few code as possible and if you need this go for @lasers proposal only with the try...except

Would that be acceptable to you?

[apiraino]

@ultrabug that's perfectly fine 👍 Please feel free to manage the issue the best way you envision, also close it if you prefer to tackle the issue from another point of view (you have the big picture here). This was just a teeny tiny issue I can live with of patch locally. I don't really care ;-)

[lasers]

@ultrabug My proposal does not work too well. The better solution would be to cancel this PR and suggest the user to log '' instead of u'' as we want to leave Python2.7 behind.

[ultrabug]

Ok pursuing on this and going @lasers way, closing here and if we want to "fix" this just do as proposed