On 03/31/2017 08:42 AM, Josef Reidinger wrote:
> On Thu, 30 Mar 2017 18:53:42 +0200
> Ancor Gonzalez Sosa <[email protected]> wrote:
>
>> This is a follow-up on this discussion
>> https://github.com/yast/yast-storage-ng/pull/184#pullrequestreview-29672717
>>
>> The mail is mainly targeted to Josef, so we continue the discussion
>> in a more persistent and prominent place than a merged pull request.
>> Don't feel guilty if you decide to ignore it. :-)
>
> Thanks for writing it.
>
>>
>> Before introducing the Y2Storage wrapper classes on top of the Storage
>> ones, we had a query API (already discussed in this list) based on the
>> concept of Y2Storage::DevicesLists.
>>
>> I think we don't need that anymore with the wrapper. I would go for an
>> approach based on pure Ruby arrays, now that we don't have to fear
>> not-found exceptions or to care about downcasting. I would simply add
>> the following convenience methods to Y2Storage::Device and that's it.
>> https://github.com/yast/yast-storage-ng/pull/184/commits/0f733850786242cddf79c7c3f8a733428d8c5674
>
> Well, my biggest problem with this approach is that ancestor have to
> know about all its kids to have comprehensive API, which is very wrong
> approach from my POV. It also have to be modified, when new child is
> introduced to have it on same level as others.
>
> It should be responsibility of caller to know specific child if he want
> it. Device responsibility is to provide methods that are same for all
> devices and makes sense for them like comparing devices, its
> display_name or its ancestors. Consider if ruby use this approach and
> Object have methods like `string?` or `integer?`, do you feel it is
> good approach? Object have `is_a?` which moves responsibility to know
> about child to caller.
>
> So I suggest to use common ruby stuff.
I'm also fine with that. Some cases will become a little bit more
verbose, but I can live with it. As I said, I just wanted those method
to shorten stuff (see below), not because they are crucial.
> [...]
>
>> 8)
>> all_pvs = the_vol_groups.map(&:lvm_pvs).flatten
>> all_pvs.reject { |pv| pv.blk_device.plain_device.disk? }
>>
>
> all_pvs = the_vol_groups.map(&:lvm_pvs).flatten
> all_pvs.reject { |pv| pv.blk_device.plain_device.is_a?(Disk) }
Well, actually in most cases it would be
all_pvs.reject {|pv| pv.blk_device.plain_device.is_a?(Y2Storage::Disk)}
>> 9)
>> !!(partition.filesystem && partition.filesystem.type.is?(:btrfs))
>>
>> 10)
>> # See notes below
>> devgraph.disks.select do |d|
>> d.filesystem == the_filesystem ||
>> d.partitions.any? {|p| p.filesystem == the_filesystem }
>> end
>>
>> # Beware, this is not equivalent because it also returns filesystems
>> in # partitions of a software RAID or any other partitionable device
>> that # is not a disk
>> the_filesystem.ancestors.select do |a|
>> (a.disk? || a.partition?) && a.filesystem == the_filesystem
>> end
>
> the_filesystem.ancestors.select do |a|
> (a.is?(Disk) || a.is_a?(Partition) && a.filesystem ==
> the_filesystem
> end
Once again, it would be slightly longer.
the_filesystem.ancestors.select do |a|
(a.is_a?(Y2Storage::Disk) || a.is_a?(Y2Storage::Partition) &&
a.filesystem == the_filesystem
end
>> 11)
>> the_filesystem.ancestors.select(&:disk?).uniq
>
> the_filesystem.ancestors.grep(Disk).uniq
>
>>
>> 12)
>> filesystem.blk_devices.select(&:disk?)
>
> filesystem.blk_devices.grep(Disk)
>
>>
>> 13)
>> filesystem.plain_blk_devices.select(&:disk?)
>>
>
> filesystem.plain_blk_devices.grep(Disk)
Same for those three. It would be grep(Y2Storage::Disk)
As said, I'm fine with using plain #is_a?, just pointing we would need
to qualify that with Y2Storage:: very often
Just thinking aloud. Would it make sense to add the same kind of
shortcuts but with a more correct approach like defining
#device?(whatever) in the base class to always return false and then
redefine it in descendant classes to allow stuff like this?
all_pvs.reject {|pv| pv.blk_device.plain_device.device?(:disk)}
# or
the_filesystem.ancestors.select do |a|
(a.device?(:disk) || a.device?(:partition) &&
a.filesystem == the_filesystem
end
If instead of #device? we call it #is?, it would look similar to the
enum wrappers and would be even shorter (but maybe too similar to #is_a?).
>> Feel free to provide more "challenges" and/or test alternative API
>> against the current ones.
>
> My challenge for you what have to changed if we add new MD Raid? So how
> hard will be to add new child to Device. This is good metric for me how
> hard/easy is to extend it.
Since we don't need to keep ABI compatibility (just API one), I don't
see it as a big challenge, neither using plain #is_a? or adding some
convenience method to the base class.
With DevicesLists:: we were adding some logic/magic on top of
libstorage, which I admit was potentially dangerous regarding the
addition of new classes. Now we are basically accessing "directly" the
libstorage API (saving us from exceptions, downcasts and enums), which
is hopefully well designed in that regard.
> Josef
>
>>
>> And yes, before you ask, it would make sense to add this to BlkDevice
>> alias_method :filesystem, :blk_filesystem
>> alias_method :direct_filesystem, :direct_blk_filesystem
>>
>> Moreover, if we find ourselves too often checking for something in a
>> disk and all its partitions, we could add a convenience method
>> Y2Storage::Partitionable#this_and_partitions.
>> Then some examples would change a bit, like:
>>
>> 10)
>> devgraph.disks.select do |d|
>> d.this_and_partitions.any? {|i| i.filesystem == the_filesystem }
>> end
>>
>> Looking forward alternatives and/or feedback.
>>
>> Cheers.
Cheers.
--
Ancor González Sosa
YaST Team at SUSE Linux GmbH
--
To unsubscribe, e-mail: [email protected]
To contact the owner, e-mail: [email protected]
