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

local: Return multiple tracks from lookup() #840

Merged
merged 3 commits into from Sep 23, 2014

Conversation

3 participants
@tkem
Member

tkem commented Aug 30, 2014

Allow local library extensions to return multiple tracks from local.Library.lookup().

See also https://discuss.mopidy.com/t/local-library-limitations/134/4

@@ -129,6 +131,26 @@ def test_lookup_unknown_track(self):
tracks = self.library.lookup('fake uri')
self.assertEqual(tracks, [])
@mock.patch.object(

This comment has been minimized.

@adamcik

adamcik Sep 3, 2014

Member

Style nitpick, this should be on one line.

@adamcik

This comment has been minimized.

Member

adamcik commented Sep 3, 2014

Could you also quickly update the json library to return a list and we can go ahead and merge this.

@tkem

This comment has been minimized.

Member

tkem commented Sep 5, 2014

Done. Also changed unit test to mock the "old" behavior returning single track.

@tkem

This comment has been minimized.

Member

tkem commented Sep 5, 2014

Could you please look into the Travis build, I get

The command "sudo apt-get install mopidy graphviz-dev" failed and exited with 100 during .
@jodal

This comment has been minimized.

Member

jodal commented Sep 5, 2014

I tried restarting the Travis build now.

@tkem

This comment has been minimized.

Member

tkem commented Sep 5, 2014

Hmmm... Doesn't seem to have helped.

@tkem

This comment has been minimized.

Member

tkem commented Sep 10, 2014

The command "sudo apt-get install mopidy graphviz-dev" failed and exited with 100 during .

WTF?

@jodal

This comment has been minimized.

Member

jodal commented Sep 10, 2014

This should have been fixed with #844.

@adamcik

This comment has been minimized.

Member

adamcik commented Sep 22, 2014

Is this blocked on anything in this branch? I've kinda lost state to be honest.

@jodal

This comment has been minimized.

Member

jodal commented Sep 22, 2014

What's the use case for converting a single URI to multiple models?

@tkem

This comment has been minimized.

Member

tkem commented Sep 23, 2014

@jodal: Use cases are discusses here: https://discuss.mopidy.com/t/local-library-limitations/134/4
@adamcik: Travis build fails due to some dependency issues. Should I rebase on current develop branch and push again?

:param string uri: track URI
:rtype: :class:`~mopidy.models.Track`
:rtype: List of :class:`~mopidy.models.Track` or single
:class:`~mopidy.models.Track` for backward compatibility

This comment has been minimized.

@jodal

jodal Sep 23, 2014

Member

I believe this line needs to be indented with four more spaces for proper formatting in the docs.

@jodal

This comment has been minimized.

Member

jodal commented Sep 23, 2014

The Travis builds was fixed with the changes done in #844. I rebuilt the job for this PR last night, and it is green now.

@jodal

This comment has been minimized.

Member

jodal commented Sep 23, 2014

@tkem, I'm open to merge this if this fixes your immediate needs.

However, I wonder if this can work more like core.library.lookup()? If you lookup an album there, you get an Album object which has a a list of all its tracks. In other words, it returns a single value, but all the data is there.

I recognize that having album or artist objects that isn't duplicated for every file in the album, we need to deduplicate them based on MusicBrainz IDs so we get a single Album object shared by all the tracks in the album. Once that's done, we'll have a nice album concept for local. Before we get there, how do you plan on using lookup() with multiple return values? Will you lookup a directory and get back Track objects for all the files in that directory, or are there other use cases I haven't thought of?

@tkem

This comment has been minimized.

Member

tkem commented Sep 23, 2014

@jodal: Please correct me if I'm wrong, but last thing I know [http://docs.mopidy.com/en/latest/api/core/#mopidy.core.LibraryController.lookup], core.library.lookup() returns a list of mopidy.models.Track.

@jodal

This comment has been minimized.

Member

jodal commented Sep 23, 2014

You're entirely right. It's just me confusing current state vs plans and core vs local.

jodal added a commit that referenced this pull request Sep 23, 2014

Merge pull request #840 from tkem/feature/local-library-lookup-multiple
local: Return multiple tracks from lookup()

@jodal jodal merged commit 1fcc75b into mopidy:develop Sep 23, 2014

1 check passed

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

@jodal jodal added this to the v0.20 - Audio cleanup milestone Sep 23, 2014

@jodal jodal self-assigned this Sep 23, 2014

@jodal jodal added the A-local label Sep 23, 2014

@tkem tkem deleted the tkem:feature/local-library-lookup-multiple branch Feb 23, 2015

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