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.*',
>
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.assertLess Equal(alloc. width, 640) Equal(alloc. height, 500) Equal(alloc. width, 640, page.module.NAME) Equal(alloc. height, 500, page.module.NAME)
>> - self.assertLess
>> + self.assertLess
>> + self.assertLess
>
> 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:
======= ======= ======= ======= ======= ======= ======= ======= ======= ======= fit_on_ a_netbook (test_gtkui. TestFrontend) ------- ------- ------- ------- ------- ------- ------- ------- ------- tdlk/canonical/ installer/ crypto/ tests/test_ gtkui.py" , line fit_on_ a_netbook assertLessEqual (alloc. height, 500, page.module.NAME)
FAIL: test_pages_
-------
Traceback (most recent call last):
File "/home/
79, in test_pages_
self.
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 \ crypto_ keys(): continuation crypto_ keys()) :
>> + not self.get_
>
> Use grouping with parentheses in preference to backslash-
> (similarly elsewhere). So:
>
> if (self.current_page == self.page_crypto and
> not self.get_
>
> No doubt we're not entirely consistent about this in existing code, but
> let's do this in new code.
>
ok.
>> + '^partman- crypto/ .*passphrase. *', crypto/ weak_passphrase ': bool(question, True, seen=False) startswith( 'partman- crypto/ passphrase' ): get_crypto_ keys(): question, self.ui. get_crypto_ keys()) crypto/ weak_passphrase $', crypto/ passphrase. *',
>
>> + elif question == 'partman-
>> + self.preseed_
>> + return True
>> +
>> + elif question.
>> + if not self.ui.
>> + return False
>> + self.preseed(
>> + 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-
> '^partman-
>
ok.
--
Regards,
Dmitrijs.