On Fri, 31 Mar 2017 10:16:34 +0200 Ancor Gonzalez Sosa <[email protected]> wrote:
> 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: > > > > [...] > > 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: [email protected] To contact the owner, e-mail: [email protected]
