Re: [yast-devel] Continued discussion on queries to the Ruby wrapper

On 03/31/2017 08:42 AM, Josef Reidinger wrote:

On Thu, 30 Mar 2017 18:53:42 +0200 Ancor Gonzalez Sosa 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/0f733850786242cddf7...

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: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org