Code review comment for lp://qastaging/~dooferlad/linaro-image-tools/fetch_image_gui

On 24 June 2011 21:58, James Westby <email address hidden> wrote:
> 133     +    if(style != None):
> 134     +        radio_button = wx.RadioButton(bind_to, label = label, style = style)
> 135     +    else:
> 136     +        radio_button = wx.RadioButton(bind_to, label = label)
>
> I think that's unnecessary isn't it? style=None is usually the default.

In this case, sadly not.

> (also we usually avoid spaces around = in method calls)

Noted and fixed.

> 226     +        if "panda" in self.settings['choice']['hardware'].keys():
> 227     +            default_hardware = "panda"
>
> Was this just to make your testing easier? I don't think that the choice should
> have a default until we're in a position to remember their last answer.

Mostly yes. I am sure we could have nothing selected and the next
button disabled by default. I didn't think it was too cheeky to do
since there are a lot of panda boards in the hands of Linaro engineers
:-) I am sure in the future we could remember some choices between
runs as well, which would be useful for just getting the latest build
for your board.

> 1562    +    def test_url_lookup(self):
>
> Please break up this test in to several tests. These sort of test cases
> are a nightmare to debug. Ideally each test method should assert a single
> thing.

I have split it into three, with functions named such that if test x
requires test y to pass before it will pass, x will run first.

--
James Tunnicliffe