> 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.
> 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.
> _proxy. account_ name = \ get(first_ key, ACCOUNT_NAME) _proxy. registration_ password = \ get(first_ key, PASSWORD) _proxy. account_ name = self._state.get( _proxy. registration_ password = self._state.get(
>
> [2]
>
> 975 + self._state.
> 976 + self._state.
> 977 + self._state.
> 978 + self._state.
>
> 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.
> first_key, ACCOUNT_NAME)
> self._state.
> 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!