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,