Code review comment for lp:~alecu/unity-lens-music/ubuntuone-webservices

Thanks for your through review!

> 56 + public unowned string sign_url2(string url, string? postargs,
>
> Looking at the docs, it says that you need to free the return, so s/unowned//,
> plus the postargs should be "out string postargs" as it's also a return (out
> params are nullable by default).

Fixed as requested.

> 118 + internal Soup.SessionAsync http_session;
>
> "internal" is a special keyword used when building libraries in vala (internal
> symbols are callable by other parts of the library, but not by the consumers),
> pls use private/public as appropriate.

Fixed all unnecessary uses of "internal". A few remain, as needed in some tests.

> 177 + public string nickname {
> 178 + get { return _nickname; }
> 179 + }
> + other instances
>
> No need for the extra private variable, use "public string x { get; private
> set; }"

Fixed as requested.

> 243 + credentials_management.find_credentials ();
> 244 + yield;
>
> Wouldn't it be better if find_credentials() itself was async? Then you
> wouldn't need the signals and you could just return the credentials / throw an
> error in the async method.

find_credentials is defined in lp:ubuntu-sso-client, and used via dbus. We found some timeout problems in the python dbus bindings that were solved by using signals instead, so most of the client code in Ubuntu SSO Client and Ubuntu One already uses this pattern. I agree that it would simplify things, but too much code depends on this dbus API as is, and it's not easy to change just for this case.

> 278 + queue_signed_webservice_call ("GET", ACCOUNT_URI,
> (session, message) => {
>
> You're using this multiple times, perhaps add a helper async method that takes
> method + uri and returns flattened Soup.MessageBody?

As suggested, I created a helper async method for the repeated instances. There are still at least two places which did not follow the same pattern (one used different http object and error codes, the other was not async), so I kept the existing code for those.

> 285 + var result = (string) message.response_body.data;
>
> .data could be null, no? Or does the flatten() cause possible null to become
> ""?

I just checked, and if .data is null, flatten() does the right thing and turns it into an empty string.

> 576 + return 0;
>
> You should return result of Test.run ()

Fixed.

> 852 + MainLoop mainloop = new MainLoop ();
> 853 + purchase_service.refetch_payment_info.begin((obj, res) => {
> 854 + mainloop.quit ();
> 855 + try {
> 856 + purchase_service.refetch_payment_info.end (res);
> 857 + } catch (PurchaseError e) {
> 858 + error ("Can't fetch payment info: %s", e.message);
> 859 + }
> 860 + });
> 861 + mainloop.run ();
>
> I remember you mentioned on IRC that you weren't sure how to do async tests
> with vala, so yep, this is the way, usually we just have a wrapper for the
> mainloop.run() which also adds a timeout, so the test fails eventually if
> there's no response. Usually using "assert (run_with_timeout (mainloop));"

Good point. I've added run_with_timeout in all this places.

Thanks!