Comment 3 for bug 1185050

Revision history for this message
David Callé (davidc3) wrote :

Hi Michael and Lukasz!
First, thanks to both for creating the bug and taking the time to do this lenghty review.

* "launchpad and sshsearch are python2-based. We shouldn't add more python2 packages to the desktop image, if we can help it. I understand that they are both only python2 because of dependencies. If the Desktop team really wants them in, I defer to them."

The Launchpad scope could maybe leave the client side to move on the smart scopes server, but that would kill the possibility of having any desktop integration with it in the future. I do agree with the rationale of keeping new python2 out, but I think it would be unfortunate to ship a GitHub scope and not a Launchpad one. The sshsearch one has a dependency on python-paramiko, and the python3 paramiko port is being discussed on their project bugtracker, but not there yet.

* "musique has one oddity in its get_album_art() function: it returns "audacious" as the icon name in a few error cases. Is that intentional?"

That's clearly a bug we missed, Musique not being one of the most used scope of the stack.

* "guayadeque has a test method (test_guayadeque_failing_search) that has a typo. It references the file mock_clementine_fail, when it should be mock_guayadeque_fail. This test should be written better to make sure it is actually loading the mock and testing what it thinks it is. This is a general problem with these scope test suites. Many have failure test cases, but they aren't very narrowly targeted. The general problem isn't a blocker, but guayadeque at least should fix the typo."

The requested fixes will likely land in a few hours in their respective trunks. I will also make sure to review tests for each scope the next time they are updated.