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

Fix library.lookup with an empty uri list #1620

Merged

Conversation

kingosticks
Copy link
Member

Fixes #1619.

@kingosticks
Copy link
Member Author

Any comments or shall I just merge?

for backend, backend_uris in self._get_backends_to_uris(uris).items():
for u in backend_uris:
futures[(backend, u)] = backend.library.lookup(u)
futures = {
Copy link
Member

Choose a reason for hiding this comment

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

I find this a lot harder to read and parse with the multiple for loops in this expansion. To the point of it being hard to follow what the real change is.

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 actually totally agree, I pretty much copied it from the other similar place it was used.

@kingosticks kingosticks force-pushed the fix/library_lookup_empty_uris branch from 9464862 to 0933af7 Compare June 14, 2017 23:05
@kingosticks kingosticks force-pushed the fix/library_lookup_empty_uris branch from 0933af7 to d6eff50 Compare June 14, 2017 23:10
@kingosticks
Copy link
Member Author

I think this is much clearer.

@@ -228,8 +228,9 @@ def lookup(self, uri=None, uris=None):

# TODO: lookup(uris) to backend APIs
for backend, backend_uris in self._get_backends_to_uris(uris).items():
for u in backend_uris:
futures[(backend, u)] = backend.library.lookup(u)
if backend_uris:
Copy link
Member

Choose a reason for hiding this comment

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

Could also be for u in backend_uris or [] I think, but either way is fine with me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hehe, Yeh, but I've gone from super obfuscated to super obvious here - no middle ground!

@kingosticks kingosticks merged commit 9554a3b into mopidy:develop Jun 15, 2017
@kingosticks kingosticks deleted the fix/library_lookup_empty_uris branch June 15, 2017 21:57
@jodal jodal added this to the v2.2 milestone Mar 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants