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

Add library.get_images() #981

Merged
merged 7 commits into from Feb 12, 2015

Conversation

2 participants
@adamcik
Member

adamcik commented Feb 12, 2015

Fixes #973 and #838 - followup will be to start deprecating the old images in models.

"""Lookup the images for the given URIs
:param list uris: list of uris to find images for
:rtype: dict mapping uris to :class:`mopidy.models.Image` instances

This comment has been minimized.

@jodal

jodal Feb 12, 2015

Member

s/uris/URIs/ in the text.

This comment has been minimized.

@jodal

jodal Feb 12, 2015

Member

It isn't clear if the mapping is from URI to Image or URI to list of Image.

This comment has been minimized.

@adamcik

adamcik Feb 12, 2015

Member

I implemented it wrong, it should be {uri: [images]} - and yes to description can also be more clear.

ref = Ref.track(uri='foo', name='bar')
self.assertEqual(ref.uri, 'foo')
self.assertEqual(ref.name, 'bar')
self.assertEqual(ref.type, Ref.TRACK)

This comment has been minimized.

@jodal

jodal Feb 12, 2015

Member

Was removing this test intentional?

This comment has been minimized.

@adamcik

adamcik Feb 12, 2015

Member

Nope, will fix.

@jodal jodal added this to the v0.20 - Audio cleanup 1 milestone Feb 12, 2015

@jodal jodal self-assigned this Feb 12, 2015

self.assertEqual({}, self.core.library.get_images(['dummy4:bar']))
def test_get_images_returns_empty_dict_for_library_less_uri(self):
self.assertEqual({}, self.core.library.get_images(['dummy3:foo']))

This comment has been minimized.

@adamcik

adamcik Feb 12, 2015

Member

Should we return the empty dict, or should we return {'dummy3:foo': []}?

This comment has been minimized.

@jodal

jodal Feb 12, 2015

Member

Latter, I think. That way we only have one "failure result".

@jodal jodal changed the title from Feature/core get images to Add library.get_images() Feb 12, 2015

@jodal jodal added the 2 - Working label Feb 12, 2015

adamcik added some commits Feb 12, 2015

core: Always return an answer for all URIs in get_images
Also make sure that results are tuples instead of lists so we don't
accidentally give out mutable state.
@adamcik

This comment has been minimized.

Member

adamcik commented Feb 12, 2015

There, that should be better.

self.library1.get_images().get.return_value = {
'dummy1:track': [Image(uri='uri1')]}
self.library1.get_images.reset_mock()
self.library2.get_images().get.return_value = {

This comment has been minimized.

@jodal

jodal Feb 12, 2015

Member

I know we've used this pattern some places in our tests, but the proper way to do this (without registering an extra call to get_images()) is to s/get_images().get/get_images.return_value.get/.

jodal added a commit that referenced this pull request Feb 12, 2015

@jodal jodal merged commit 6387405 into mopidy:develop Feb 12, 2015

2 checks passed

Scrutinizer 15 updated code elements
Details
continuous-integration/travis-ci The Travis CI build passed
Details

@jodal jodal added 3 - Done and removed 2 - Working labels Feb 12, 2015

@adamcik adamcik deleted the adamcik:feature/core-get-images branch Feb 12, 2015

@jodal jodal removed the 3 - Done label Feb 13, 2015

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