On 03/30/2017 06:53 PM, 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. :-)
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.
So I decided to do another test to validate that idea. Let's take existing code using the DeviceLists superpowers and compare how it would look like with plain arrays of wrapped objects. 1) # current efi_partition = devgraph.blk_filesystems.with_mountpoint("/boot/efi").partitions.first # wrapper + plain arrays efi_partition = devgraph.partitions.detect do |p| p.blk_filesystem && p.blk_filesystem.mountpoint == "/boot/efi" end 2) # current mbr_disk = devgraph.disks.with(name: mbr_disk_name).first # wrapper + plain arrays mbr_disk = devgraph.disks.detect {|d| d.name == "mbr_disk_name" } 3) # current partitions = devgraph.disks.with(name: disk.name).partitions.reject do |part| [Storage::ID_SWAP, Storage::ID_GPT_BIOS].include?(part.id) end # wrapper + plain arrays partitions = devgraph.partitions.select |part| part.disk && part.disk.name == disk.name && ![:swap, :gpt_bios].include?(part.id.to_sym) end 4) # current def part_or_disk_at_mountpoint(mntpnt) fses = staging.filesystems.with_mountpoint(mntpnt) return nil if fses.empty? partitions = fses.partitions partitions = fses.lvm_vgs.partitions if partitions.empty? return partitions.first unless partitions.empty? disks = fses.disks disks = fses.lvm_vgs.disks if disks.empty? disks.first end # wrapper + plain arrays def part_or_disk_at_mountpoint(mntpnt) fs = staging.filesystems.detect {|f| f.mountpoint == "mntpnt"} return nil unless fs fs.ancestors.detect {|a| a.partition? } || fs.ancestors.detect {|a| a.disk? } end Those four examples should be enough as an exercise. In general it looks like more code because we don't have the #with shortcut (it was useful as a shortcut, but its real motivation was to deal with Storage un-rubistm), but I still think it's ok since now is straightforward common Ruby code with no magic. -- 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