[blivet:f21-branch/master 0/3] #1150147 patches (v2)

David Lehman dlehman at redhat.com
Fri Oct 10 17:06:51 UTC 2014


On 10/10/2014 10:25 AM, mulhern wrote:
> This is a completely different approach to the previous patch.
>
> It's an alternative, rather than superceding it.
>
> I think patch (1) should go into master regardless of whether this solution
> is preferred.
>
> Patch (2) is kind of long winded and ugly because it is necessary to check
> parents, not self.parents, and although self.parents is guaranteed to
> be a ParentList, parents could be a lot of things.
>
> Patch (3) is just tests, like before.
>
> There is an alternative (v3) where the logic in patch (2) goes into
> MDRaidArrayDevice constructor before the superclass call.
> Instead of changing formatClassName, self._formatClassName is changed.
> That idea would be slightly preferable to this one, except that, currently
> self._formatClassName can not be changed, because it is a property.
> So, it would have to be extended with a setter as well as a getter,
> which set some instance variable of MDRaidArrayDevice invented for just
> this purpose.

Ah, yes. I forgot how difficult this is made by the use of 
abc.abstractproperty. It would be so simple if those were normal class 
attributes.

It would be simple enough to add a minimal setter, but it starts to feel 
too heavy for what could easily be a simple class attribute.

An advantage of what you call v3 that you didn't mention is containing 
the madness of MDRaidArrayDevice within that class instead of spreading 
it around. I really dislike having generic classes reference specific 
classes like patch 2 does. I'm willing to go pretty far to avoid it.

I tend to lean toward changing the abstract properties into normal class 
attributes and proceeding with your v3. Failing that, I think I prefer 
v1 to v2.

Also note that this circular dependency you are introducing between 
ContainerDevice and MDRaidArrayDevice will be a problem on master.

>
> There is another alternative (v4) where an MDRaidArrayDevice gets its
> format set to mdmember if it is a container. The worry about that is
> that that wouldn't be a good representation of reality, and yet might
> be interpreted as such in other code.
>
> The drawback of (v2) and (v3) is that they both require certain implicit
> facts to hold true, e.g., that if an MDRaidArrayDevice has an mdcontainer
> parent, then it has only one parent. The programmer has to understand
> these facts to understand why the code works.

You could use all() and avoid caring about how many parents there are.

David

>
> (v1) uses class inheritance, which is a heavyweight solution, but it
> requires very little of the programmer (or reader's) understanding and seems
> more robust, so I like it better. (v1) adds fewer lines of actual code than
> either of (v2) or (v3) would require.
>
> (v1) could be adapted so that, instead of accepting any instance of
> an mdcontainer, regardless of its format, it at least checks that the
> format is a subtype of DeviceFormat. It would still be shorter than
> (v2) or (v3).
>
> mulhern (3):
>    Register DeviceFormat class (#1150147)
>    Don't check for mdmember format when parent is container (#1150147)
>    Update tests to bring into line w/ previous commits (#1150147)
>
>   blivet/devices.py          | 24 ++++++++++++++++++++++--
>   blivet/formats/__init__.py |  2 ++
>   tests/devices_test.py      | 10 ++++------
>   3 files changed, 28 insertions(+), 8 deletions(-)
>



More information about the anaconda-patches mailing list