Mailinglist Archive: yast-devel (63 mails)

< Previous Next >
Re: [yast-devel] RFC - New Storage API
On Fri, Dec 05, 2014 at 02:06:26PM +0100, Martin Vidner wrote:
On Wed, Dec 03, 2014 at 11:07:34AM +0100, Arvin Schnell wrote:
Bindings
--------

I have swig generated bindings for Ruby, Python and Perl5. The repository
contains a few examples for each of them:

https://github.com/aschnell/libstorage-bgl-eval/tree/master/bindings

Hi Martin,

thanks for your review.

The method naming convention, in camelCase, looks out of place in
Ruby. It should be possible to autoconvert it to snake_case via
swig -ruby -autorename example.i
http://www.swig.org/Doc3.0/Ruby.html#Ruby_nn27

- sda1 = gpt.createPartition("/dev/sda1")
+ sda1 = gpt.create_partition("/dev/sda1")

I have enabled autorename for Ruby.

Similarly, though less automatable, it would be good to rename
getters and setters:
http://www.swig.org/Doc3.0/Ruby.html#Ruby_nn31

- partition.getNumber()
+ partition.number

Well, that's indeed a lot of work. Having to take care of three
bindings I think it's too much. Also I want to keep the SWIG
interface file identical for all three bindings if possible.

Abbreviations can be useful to save typing and screen space but IMHO
they should be avoided in class names:

- Storage::BlkDevice::find(...)
+ Storage::BlackDevice.find(...)

In general I agree but "blk" is very common in the storage are,
e.g. S_ISBLK and BLKGETSIZE in C headers or lsblk and blkid on
the command line.

Small improvements in style in the examples:

- Storage::Disk::create(device_graph, "/dev/sda")
+ Storage::Disk.create(device_graph, "/dev/sda")

- print "foo\n"
+ puts "foo"

OK.

Please avoid booleans in the API, they are hard to understand even
if you have learned the domain and the API before.
I haven't seen a single mention od Ruby symbols in the Swig docs,
I guess custom typemaps would be needed for that :-(

- sda.getDescendants(false)
+ sda.descendants(:direct) # or Storage::DESCENDANTS_DIRECT

Looks difficult to achieve.

For polymorphism, we have these 6 casts in Storage::Device:
(which return nil if the instance is not of that type)
#castToDisk
#castToPartitionTable
#castToPartition
#castToLvmVg
#castToLvmLv
#castToFilesystem
While the ideal OO design is to avoid casts and use method
polymorphism, I don't know how doable it is in this case.

Downcasting should not be needed often since querying e.g. the
partitions of a partition table of course returns pointers to
partitions not devices. The casts are only needed when you do a
generic query on the device graph and then want to know more
about the returned devices.

Just a note: Using the class name instead of downcasting is
wrong. E.g. there could be three distinct classes for partitions
on MSDOS, GPT and DASD instead of just one. Code using

if device.get_class_name == "Partition"

will then fail if device is actually "GptPartition" while

if to_partition(device)

will still work. I will add this note to the final documentation
since I want the freedom to change such things later on.

A more
rubyish method names would be
#to_disk
#to_partition_table
#to_lvm_vg

I have removed the "cast" from the names to together with
autorename it's what you want now.

Regards,
Arvin

--
Arvin Schnell, <aschnell@xxxxxxx>
Senior Software Engineer, Research & Development
SUSE LINUX GmbH, GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer, HRB 21284
(AG Nürnberg)
Maxfeldstraße 5
90409 Nürnberg
Germany
--
To unsubscribe, e-mail: yast-devel+unsubscribe@xxxxxxxxxxxx
To contact the owner, e-mail: yast-devel+owner@xxxxxxxxxxxx

< Previous Next >
Follow Ups