Merge lp:~diegosarmentero/ubuntuone-client/crazy-comma into lp:ubuntuone-client

[Diego Sarmentero]

- The comma was misplaced.

[Brian Curtin (brian.curtin)]

review: Approve

[Mike McCracken (mikemc)]

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

[Natalia Bidart (nataliabidart)]

Completely agreed with Mike, the comma is correct (though I agree we could improve the readability of the code).

The deferred's result is expected to be a tuple, in all cases (within this module). What error were you trying to solve?

Thanks.

[lp:~diegosarmentero/ubuntuone-client/crazy-comma]

1331. By Diego Sarmentero on 2012-10-05

adding comment

[Diego Sarmentero (diegosarmentero)]

> 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…

adding a comment specifying the need of the comma, and fixed the u1-cp part

[Mike McCracken (mikemc)]

OK, comment seems useful.

review: Approve

=== modified file 'ubuntuone/platform/tools/__init__.py'
--- ubuntuone/platform/tools/__init__.py	2012-08-06 20:01:04 +0000
+++ ubuntuone/platform/tools/__init__.py	2012-10-05 12:46:23 +0000
@@ -504,7 +504,7 @@
         """Get the public files list."""
         d = self.wait_for_signals('PublicFilesList', 'PublicFilesListError')
         self.proxy.call_method('public_files', 'get_public_files')
-        response, = yield d
+        response, = yield d  # unpacking single element tuple
         defer.returnValue(response)
 
     @defer.inlineCallbacks