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

Some fixes for non-ASCII chars in MPD uris #1805

Merged
merged 2 commits into from Oct 1, 2019

Conversation

@kingosticks
Copy link
Member

kingosticks commented Sep 26, 2019

Fixes #1759

Most of this comes from Track uris being bytes but falling over when formatted with unicode strings for output / logging. And then I went a bit mad sprinkling non-ASCII throughout the tests.

@kingosticks kingosticks added the A-mpd label Sep 26, 2019
@kingosticks kingosticks requested a review from jodal Sep 26, 2019
@kingosticks kingosticks added this to the v2.3.0 milestone Sep 26, 2019
@codecov

This comment has been minimized.

Copy link

codecov bot commented Sep 26, 2019

Codecov Report

Merging #1805 into release-2.2 will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@              Coverage Diff               @@
##           release-2.2   #1805      +/-   ##
==============================================
+ Coverage        80.99%     81%   +<.01%     
==============================================
  Files               83      83              
  Lines             6647    6649       +2     
==============================================
+ Hits              5384    5386       +2     
  Misses            1263    1263
Impacted Files Coverage Δ
mopidy/mpd/protocol/stored_playlists.py 90.9% <100%> (ø) ⬆️
mopidy/mpd/translator.py 95.4% <100%> (+0.1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b66c2e8...8b45292. Read the comment docs.

@jodal jodal force-pushed the mopidy:release-2.2 branch from 5bcb4ca to bc3efdb Sep 30, 2019
@jodal jodal changed the base branch from release-2.2 to release-2.3 Sep 30, 2019
@jodal
jodal approved these changes Sep 30, 2019
@@ -49,6 +49,7 @@ def get_distinct(self, field, query=None):
return self.dummy_get_distinct_result.get(field, set())

def lookup(self, uri):
uri = Ref.track(uri=uri).uri

This comment has been minimized.

Copy link
@jodal

jodal Sep 30, 2019

Member

This looked like a no-op to me at first, but this actually encodes the URI as bytes!

This comment has been minimized.

Copy link
@kingosticks

kingosticks Oct 1, 2019

Author Member

Yes! It's so tempting to just compare two stringy things called uri but you can't.

@@ -22,6 +22,7 @@ def __init__(self, *args, **kwargs):
self.response = []

def queue_send(self, data):
data = data.decode('utf-8')

This comment has been minimized.

Copy link
@jodal

jodal Sep 30, 2019

Member

data is one small step away from being sent over the network, so it wouldn't be unnatural to keep it as bytes. If we change this to the following, it should work correctly on both Python 2 and 3:

if isinstance(data, compat.text_type):
    data = data.encode('utf-8')
lines = (line for line in data.split(b'\n') if line)  # Note the explicit 'b' here
...

This comment has been minimized.

Copy link
@kingosticks

kingosticks Oct 1, 2019

Author Member

OK, sounds sensible. Will update.

This comment has been minimized.

Copy link
@kingosticks

kingosticks Oct 1, 2019

Author Member

Hmm but actually, no. This isn't going over the network and don't we want unicode here so that we can just write unicode in the tests?

@jodal jodal merged commit dd47db4 into mopidy:release-2.3 Oct 1, 2019
3 checks passed
3 checks passed
codecov/patch 100% of diff hit (target 80.99%)
Details
codecov/project 81% (+<.01%) compared to b66c2e8
Details
test Workflow: test
Details
@kingosticks

This comment has been minimized.

Copy link
Member Author

kingosticks commented Oct 1, 2019

kingosticks added a commit to kingosticks/mopidy that referenced this pull request Oct 1, 2019
kingosticks added a commit to kingosticks/mopidy that referenced this pull request Oct 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.