Code review comment for lp://staging/~tealeg/landscape-client/ui-interface-iteration3

Revision history for this message
Geoff Teale (tealeg) wrote :

> Looks great!
>
> [1]
>
> 427 + except StateError:
> 428 + # We probably don't care.
> 429 + pass
>
> Even if we don't care, maybe we should log something instead of not handling
> it?

Actually this error is just telling us that there are no changes to save, originally I had the idea that we would disable the OK button until changes were made, but this is generally a bad thing from a UX point of view, so instead I just catch the error and push on. In the later branch this results in a registration attempt.

Now logging as suggested.

>
>
> [2]
>
> 975 + self._state._proxy.account_name = \
> 976 + self._state.get(first_key, ACCOUNT_NAME)
> 977 + self._state._proxy.registration_password = \
> 978 + self._state.get(first_key, PASSWORD)
>
> I've noticed in previous reviews that we should avoid breaking likes using
> backslash. Maybe the following could make it consistent with the rest of our
> codebase.
>
> self._state._proxy.account_name = self._state.get(
> first_key, ACCOUNT_NAME)
> self._state._proxy.registration_password = self._state.get(
> first_key, PASSWORD)
>
> (Didn't check if that goes beyond the 79char limit)

Done!

>
> Also, do you think it would make sense to move l.ui.tests.helpers to a
> l.ui.testing module so it's more consistent with what we have in the server?
> No biggie on this one, just trying to keep consistency at all costs :)

I'd rather be consistent within the client for now and maybe we can convert all cases as a separate task.

> +1!

« Back to merge proposal