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

Revision history for this message
James Westby (james-w) 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.

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

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.

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.

Thanks,

James

« Back to merge proposal