Code review comment for lp:~milo/linaro-image-tools/bug1073973

Revision history for this message
Stevan Radaković (stevanr) wrote :

> Thanks Stevan for looking into it!
>
> On Tue, Feb 12, 2013 at 10:04 AM, Stevan Radaković
> <email address hidden> wrote:
> > Hey Milo, this looks good,
> >
> > The diff part with method removal of set_board confused me a bit, didn't see
> anything about it in the description
>
> Yeah... for me that was part of the minor refactorings, could have
> marked that more promptly though. :-/
> I removed it because it was not strictly necessary, since there is not
> real data hiding and the property was not "internal" to the class, it
> can be easily accessed without the function.

Cool, clear..

>
> > One question regarding
> >
> > 86 +
> > 87 + bootloader = property(_get_bootloader, _set_bootloader)
> >
> > Shouldn't we put this property definition on the top of the class? it really
> looks strange being in the middle of method definition...
>
> I'm not following here... the property definition is after the
> __init__ and after the two property methods (_get and _set). I cannot
> move it before those or at the top of the class, or when accessing it
> Python will complain that the methods are not defined.
> We would have to move everything, the property and the methods, before
> the __init__, but from a logical point of view I would say to keep
> them after the __init__.
>

Oh ok I wasn't aware of this, thought you can define property first and then methods later.
Thanks for explaining.

Anyway, long overdue, approve +1 :)

> Ciao.
>
> --
> Milo Casagrande | Infrastructure Team
> Linaro.org <www.linaro.org> │ Open source software for ARM SoCs

review: Approve

« Back to merge proposal