Code review comment for lp:~milo/linaro-image-tools/boards-refactoring
Revision history for this message
| Milo Casagrande (milo) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
73416ba
(Get the code!)
> No reason was given for going from class to instance attributes, neither in
> commit message, nor in MR. Why would that matter?
You are right, I just discussed this with Danilo.
The problem is due to the introduction of the Android hwpack: a config file that needs to be parsed, and whose values have to be stored in the already defined board classes (both in AndrodiBoardConfig and in BoardConfig, since the former inherits from the latter and uses methods defined in its own and also from the parent one).
The problem I had with the first easy implementation (just parse the file and use everything as it was), was that it was not working, and tests were failing simply because the values were not correctly stored and the "default" ones were used. After a little bit of investigation and trying with different approaches, the "easiest" solution would have been to switch to instance attributes and methods instead of class based ones.
This is due to the fact that I wanted to have an easy way to set-up the attribute values, moslty using getattr and setattr, that work only with instances, not classes. In the old code, the attribute or properties where custom-defined as "classproperty", something that python does not have natively: keeping that approach would have meant an hack to be able to properly set the "classproperty" (not really an hack, but something that doesn't feel natual and definitely not easy to implement: we would have had to use "metaclasses"), since the get part is the only one to work correctly, and defining a class-based setter is close to impossible.
> And character content of
> changes is massive, typo can lurk in easily. Merging this without passing
> tests is a bit brave IMHO.
You are right, but if I fix the tests in the same MP, it will be even bigger than it is now (and there will be that complaint :-P). The tests have been fixed and the branches should be applied one on top of the other until the last one. I know it is sub-optimal, but at least changes have been split out into "smaller" review chunks (even if they result in more than 2000 lines). If I put the test fixes here, I will add almost 3000 more lines.