Merge lp://qastaging/~milo/linaro-image-tools/boards-refactoring into lp://qastaging/linaro-image-tools/11.11

Proposed by Milo Casagrande
Status: Merged
Merged at revision: 596
Proposed branch: lp://qastaging/~milo/linaro-image-tools/boards-refactoring
Merge into: lp://qastaging/linaro-image-tools/11.11
Prerequisite: lp://qastaging/~milo/linaro-image-tools/hwpack-handler
Diff against target: 2224 lines (+773/-771)
2 files modified
linaro-media-create (+3/-3)
linaro_image_tools/media_create/boards.py (+770/-768)
To merge this branch: bzr merge lp://qastaging/~milo/linaro-image-tools/boards-refactoring
Reviewer Review Type Date Requested Status
Paul Sokolovsky Approve
linaro-image-tools maintainers Pending
Review via email: mp+138228@code.qastaging.launchpad.net

Commit message

Boards class refactoring and clean-up.

Description of the change

Second, and a little big, merge request.

This is the initial refactoring work of the boards module moving from class methods to instance methods.
Some code needs further clean-up. In this branch tests have not been fixed.

To post a comment you must log in.
592. By Milo Casagrande

Merged hwpack-handler into boards-refactoring.

593. By Milo Casagrande

Merged from trunk.

594. By Milo Casagrande

Readedd wrongly removed attribute.

Revision history for this message
Paul Sokolovsky (pfalcon) wrote :

No reason was given for going from class to instance attributes, neither in commit message, nor in MR. Why would that matter? And character content of changes is massive, typo can lurk in easily. Merging this without passing tests is a bit brave IMHO.

review: Abstain
Revision history for this message
Milo Casagrande (milo) wrote :

> 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.

Revision history for this message
Paul Sokolovsky (pfalcon) wrote :

Thanks for explanation, makes sense to clean up that mess.

If tests are updated in following merge and pass, then it looks good.

review: Approve
595. By Milo Casagrande

Merged from trunk.

596. By Milo Casagrande

Refactored OrigenQuad to be an instance.

597. By Milo Casagrande

Merged from trunk.

598. By Milo Casagrande

Merged from trunk.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches