On 02/24/2017 10:37 AM, Josef Reidinger wrote:
> On Thu, 23 Feb 2017 17:45:48 +0100
> Ancor Gonzalez Sosa <[email protected]> wrote:
>
>> My Hack Week project is in that state in which all the important parts
>> work and now it's time to decide if it's worth investing a couple of
>> extra days to finish it all the way down.
>>
>> Since the ultimate goal is to change how YaST interacts with
>> storage-ng, I would need the opinion of as many YaST developers as
>> possible.
>>
>> So please, please, please, take a look to this and give your opinion.
>> https://github.com/yast/yast-storage-ng/pull/169
>>
>> The most important question is - do you prefer the proposed approach
>> and API* or do you prefer to continue accessing directly to
>> libstorage-ng from YaST?
>>
>> Thanks
>>
>> * In general, minor details in the API can be ironed out in the short
>> future.
>
> let me comment it point by point:
>
> Arrays instead of strictly-typed vectors
> ----------------------------------------
>
> I think it make sense to have a bit specialized arrays, but it do not
> make sense to not act as array, so for me the best way would be to have
> something like `class MountPoints < Array` which have special filters
> like mountpoints.snapshots which return again MountPoints, but only
> btrfs snapshots. This allow easy extending in future and still it act
> like array.
We have DevicesList and all its subclasses for that (already presented
in a mail some time ago, usually referred as "query API" and extensively
used in yast2-bootloader, for example). But since that was already
provided by the current yast2-storage-ng, I wanted to leave it out of
the discussion. With this new API, DevicesList & friends would still be
there but MUCH, MUCH better integrated (no more tricks or refinements).
For example, you could do things like
dvcegraph.all_filesystems.with_mountpoint("/boot")
disk.all_partitions.with {|p| p.size > 10.GiB && p.boot? }.map(&:name)
dvcegraph.all_disks.with(name: /vd/).all_partitions.with(number: 1)
> No more downcasting
> -------------------
>
> good improvement
>
>
> Nil instead of exceptions
> -------------------------
>
> make sense for me, another option that make sense for me is NullObject,
> that allows some introspection and e.g. better writting to log.
>
> Classes instead of constants and enums
> -------------------------------------
>
> nice improvement
>
> All sizes as DiskSize objects
> -----------------------------
>
> makes sense as it is not only integer, but have its own logic
>
>
> Adding methods needed by YaST
> -----------------------------
>
> makes sense
>
>
> Less surprising API (for Rubyists)
> ----------------------------------
>
> nice just one more idea. I really hate to type everywhere staging
> graph. So why not have `def all(graph = Storage.staging)` as I think
> majority of other Yast modules are interested mainly in staging graph.
Well, being picky it would be
def all(graph = StorageManager.instance.staging)
but ok. :-)
> Overall I like it.
>
> Josef
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]
