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

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

Hi Federico, you'll see that I added some tests but haven't filled them in.

While back-filling the tests but stopped short of actually implementing them; mainly because the way you've made the fix is different to what I originally had and my initial thoughts wouldn't work :-)

Could you take a look at writing the tests? My current concern with the current implementation is that _combine_base_and_extensions has to know about class stuff (__bases__ and mro) where as the previous method only dealt with tuples of information.
Not only does this mean that it didn't need to know more than 'merge these tuples' it was easy to write tests for (again, just dealing with tuples if data, could be strings etc.).

Also, without a test in place I'm not sure that the sorting of the mro is the correct thing to do.
While I didn't have a test in place to prove it, the intention of the purely tuple approach is that it kept the expected order of merging 2 bases.

« Back to merge proposal