On Fri, 31 Mar 2017 10:16:34 +0200
Ancor Gonzalez Sosa
On 03/31/2017 08:42 AM, Josef Reidinger wrote:
On Thu, 30 Mar 2017 18:53:42 +0200 Ancor Gonzalez Sosa
wrote:
[...] 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)}
Yeah, I mentioned it on top.
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
or it can be more generic like ` a.respond_to?(:filesystem) && a.filesystem == the_filesystem which will find even encrypted devices ;)
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?).
yes, it is fine approach from my POV. If it is is? and consistent with enums, even better. It is shorter and also keep responsibility on caller. maybe we can use method `type` so we can use `[:disk, :partition].include?(a.type)`
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.
good, so I will look forward for adding it and commenting if changes looks too big :) Josef -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org