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

mpd: Ignore empty parts in query #758

Merged
merged 4 commits into from Jun 22, 2014

Conversation

3 participants
@trygveaa
Member

trygveaa commented Jun 21, 2014

This ignores all query parts with an empty value, e.g. Date ""

This is to fix e.g. that an empty string can't be parsed as a date, which may be attempted if the query contains Date "". Mopidy-Spotify tries to do this and raises an exception. IMO it's better to ignore empty parts from the MPD frontend than to let empty parts be sent to the backends.

(The development version of ncmpcpp sends a find query containing Date "" when it looks up an album it doesn't know the date for.)

trygveaa added some commits Jun 21, 2014

mpd: Ignore empty parts in query
This is to fix e.g. that an empty string can't be parsed as a date,
which may be attempted if the query contains 'Date ""'.
@jodal

This comment has been minimized.

Member

jodal commented Jun 21, 2014

LGTM

@@ -41,7 +41,9 @@ def _query_from_mpd_search_parameters(parameters, mapping):
raise exceptions.MpdArgError('incorrect arguments')
if not parameters:
raise ValueError
query.setdefault(field, []).append(parameters.pop(0))
value = parameters.pop(0)
if value:

This comment has been minimized.

@adamcik

adamcik Jun 21, 2014

Member

Should this also perhaps catch just whitespaces, so value.strip() instead?

This comment has been minimized.

@trygveaa

trygveaa Jun 21, 2014

Member

Yeah, that seems reasonable.

@@ -41,7 +41,9 @@ def _query_from_mpd_search_parameters(parameters, mapping):
raise exceptions.MpdArgError('incorrect arguments')
if not parameters:
raise ValueError
query.setdefault(field, []).append(parameters.pop(0))
value = parameters.pop(0)
if value.strip():

This comment has been minimized.

@jodal

jodal Jun 21, 2014

Member

Can you add a test case which exercises the strip()?

This comment has been minimized.

@trygveaa

jodal added a commit that referenced this pull request Jun 22, 2014

@jodal jodal merged commit 6b97f0a into mopidy:develop Jun 22, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@jodal jodal self-assigned this Jun 22, 2014

@trygveaa trygveaa deleted the trygveaa:fix/ignore-empty-query-parts branch Jun 22, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment