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

core: Make lookup() ignore tracks without URI #1381

Merged
merged 2 commits into from Jan 3, 2016

Conversation

jodal
Copy link
Member

@jodal jodal commented Jan 1, 2016

Fixes #1340

@jodal jodal added the A-core Area: Core layer label Jan 1, 2016
@jodal jodal added this to the v1.1.2 - Bugfixes milestone Jan 1, 2016
@@ -236,7 +236,7 @@ def lookup(self, uri=None, uris=None):
result = future.get()
if result is not None:
validation.check_instances(result, models.Track)
results[u] = result
results[u] = [r for r in result if r.uri]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a note or TODO to revisit this if we make the URI mandatory. So maybe just # Silently drop tracks without URIs as this crashes some clients. And on a side note should we log what backend sent the bad data?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm adding a note about making Track.uri mandatory instead. When we do that, backend's will fail to create the broken objects and will thus quickly get feedback and get fixed. Don't want to add too much logic in an inner loop.

@adamcik
Copy link
Member

adamcik commented Jan 1, 2016

Other than that optional comment this looks good to merge.

jodal added a commit that referenced this pull request Jan 3, 2016
core: Make lookup() ignore tracks without URI
@jodal jodal merged commit b2860eb into mopidy:release-1.1 Jan 3, 2016
@jodal jodal deleted the fix/1340 branch January 3, 2016 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-core Area: Core layer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants