Code review comment for lp:~canonical-platform-qa/autopilot/cpo-base-classes-fix

Revision history for this message
Christopher Lee (veebers) wrote :

Looking good guys, just a couple of comments inline.

As per the changes I made to day; I was thinking about how to handle detecting and choosing the correct base class when there was multiple inheritance involved (i.e. multiple CPO inheritance).

I came to the conclusion that it is bad form to change things under the cover in this fashion as it can be misleading and cause unexpected changes.
Instead I've opted for issuing a warning when this is detected with the option to upgrade this to raising an error in the new future (i.e. when we can be certain we don't break everyone with this new enforcement rule.)

I was of 2 minds making it raise an exception straight off, but I feel that it was to heavy handed at this stage.
What do you guys think?

I've done some small scale testing locally and the address book and unity8 tests pass as expected. I would love to see this go through the gatekeeper to see the results there.

« Back to merge proposal