Code review comment for lp:~diegosarmentero/ubuntuone-client/crazy-comma

Revision history for this message
Mike McCracken (mikemc) wrote :

The corresponding test fails for me on linux:

[ERROR]
Traceback (most recent call last):
  File "/usr/lib/python2.7/dist-packages/twisted/internet/defer.py", line 1039, in _inlineCallbacks
    result = g.send(result)
  File "/home/mmccrack/Documents/src/crazy-comma/tests/platform/test_tools.py", line 753, in test_get_public_files
    self.assertEqual(pf['node_id'], node_id)
exceptions.TypeError: list indices must be integers, not str

tests.platform.test_tools.TestToolsSomeMore.test_get_public_files

I'm also not sure this is the best way to fix this. It's not inconsistent - 8 other places in this file unpack their result like this. Why do they do that? (Their return value isn't used anywhere I could find, but I wasn't sure I was testing libsyncdaemon correctly, so it might be used there, or in other clients of this code.) Either we should change them all or leave this in and change the calling code, but without knowing the reason I really don't want to make them inconsistent.

The code that uses this return value over in ubuntuone-control-panel works just fine if you use the return value directly, eg - when you do:
result=yield self.backend.get_public_files()
then do this:
_load_public_files(result)
instead of this:
_load_public_files(result[0])

and the control-panel tests pass with that change.

Disapprove seems harsh, but I'm just not convinced that this change is necessary, or that we completely understand whether it breaks anything…

review: Disapprove

« Back to merge proposal