Code review comment for lp:~nataliabidart/ubuntu-sso-client/unify-signal-broadcaster

> * We do the following several times:
> 'SSOLogin: emitting CaptchaGenerated with app_name "%s" and result %r',
> app_name, result
> 'SSOLogin: emitting CaptchaGenerationError with app_name "%s" and error %r',
> app_name, error
>
> is it possible to be smarter regarding the logging of the DBusSignals? an idea
> would be to use a decorator that way we know the function name and the args. I
> think it will remove some noise form the code.

Yes, I added a lg_call function to the logger module to facilitate this.

> * I'm confused, why do we have the following?:
>
> 1218 + d.addCallback(lambda a: reply_handler(*a))
>
> If a is not a sequence we are going to have a runtime error..

There will be no runtime error because of the following. That code is inside the implementation of "call_method", which makes an async call thru dbus.

So, as you can see here:

    390 self.bus.call_async(
    391 bus_name=self.bus_name, object_path=self.path,
    392 dbus_interface=self.interface, method=method_name,
    393 signature=None, args=args,
    394 reply_handler=lambda *a: d.callback(a),
    395 error_handler=d.errback)

the dbus async called will call d.callback(a) when the call is finished, and because of the lamda *a, 'a' will always be a tuple (perhaps the empty one).

This method also has to call reply_handler if it was passed as named parameter, so that's why we add this code:

    382 reply_handler = kwargs.get('reply_handler', None)
    383 if reply_handler is not None:
    384 d.addCallback(lambda a: reply_handler(*a))

Since d is callback'd with a tuple, we "desrefence" that tuple for reply_handler, which expects separated arguments.

> * There are several tests of the type:
>
> 2869 + def test_credentials_cleared(self):
> 2870 + """The CredentialsCleared signal."""
> 2871 + self.obj.CredentialsCleared(APP_NAME)
> 2872 + msgs = (self.obj.__class__.__name__,
> 2873 + self.obj.CredentialsCleared.__name__, APP_NAME)
> 2874 + self.assertTrue(self.memento.check_info(*msgs))
>
> Where we call a signal, build the msg and assert, can those be merged?

Yes, I added a method called assert_refcounter_increased which unifies them all.

> There seems to be some repetitive code here:
>
> 2583 + def test_credentials_found_when_ref_count_is_not_positive(self):
...

Yes, also unified all tests you listed using assert_refcounter_decreased and assert_refcounter_negative.

Thanks for the great review!