Code review comment for lp:~xnox/ubiquity/crypto-done

On 01/08/12 15:46, Colin Watson wrote:
> This basically looks good, thanks! I confess I haven't gone through all
> the state machines in fine detail or anything, but just a few nits and
> then I think you should go ahead and merge this so that we can iterate
> quickly.
>
>> +Type: Text
>
> Lower-case "text" in all these "Type:" lines.
>

ok.

>> + <span foreground="darkred">Warning:</span> If you lose this security key, all data will be lost. if you need to, write down your key and keep it in a safe place elsewhere.
>
> Capital letter at the start of sentences ("If you need to, ..."). Same
> in the .ui file.
>

ok.

>> - self.assertLessEqual(alloc.width, 640)
>> - self.assertLessEqual(alloc.height, 500)
>> + self.assertLessEqual(alloc.width, 640, page.module.NAME)
>> + self.assertLessEqual(alloc.height, 500, page.module.NAME)
>
> page.module.NAME doesn't seem like a helpful message on test failure.
> At least include the expected and observed values.
>

It already does, by default. So it now looks like this:

======================================================================
FAIL: test_pages_fit_on_a_netbook (test_gtkui.TestFrontend)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/tdlk/canonical/installer/crypto/tests/test_gtkui.py", line
79, in test_pages_fit_on_a_netbook
    self.assertLessEqual(alloc.height, 500, page.module.NAME)
AssertionError: 593 not less than or equal to 500 : language

----------------------------------------------------------------------

Which appears to be a peculiar fill packing bug which is visible when
manually starting ubiquity with a custom build, but not visible in
during the welcome screen. Will look into this more later.

>> + if self.current_page == self.page_crypto and \
>> + not self.get_crypto_keys():
>
> Use grouping with parentheses in preference to backslash-continuation
> (similarly elsewhere). So:
>
> if (self.current_page == self.page_crypto and
> not self.get_crypto_keys()):
>
> No doubt we're not entirely consistent about this in existing code, but
> let's do this in new code.
>

ok.

>> + '^partman-crypto/.*passphrase.*',
>
>> + elif question == 'partman-crypto/weak_passphrase':
>> + self.preseed_bool(question, True, seen=False)
>> + return True
>> +
>> + elif question.startswith('partman-crypto/passphrase'):
>> + if not self.ui.get_crypto_keys():
>> + return False
>> + self.preseed(question, self.ui.get_crypto_keys())
>> + return True
>> +
>
> I'd like to see these two sections match to avoid later problems if the
> set of questions asked changes, so I think your entries in the question
> list should preferably be:
>
> '^partman-crypto/weak_passphrase$',
> '^partman-crypto/passphrase.*',
>

ok.

--
Regards,
Dmitrijs.