[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