On 10/09/2015 02:54 PM, Arvin Schnell wrote:
On Wed, Oct 07, 2015 at 02:12:44PM +0200, Ancor Gonzalez Sosa wrote:
[,,,]
Only some (maybe too Ruby-centric) comments follow.
Casting and Storage.some_method ===============================
Things like this look quite un-ruby:
tmp1 = devicegraph.find_device(42) assert(Storage.disk?(tmp1)) assert(Storage.to_disk(tmp1)) assert(!Storage.partition_table?(tmp1)) assert_raises(Storage.DeviceHasWrongType { Storage.to_partition_table(tmp1) }
I would have expected things like
tmp1.disk? tmp1.to_partition_table
At least in C++ such a interface looks like a very bad idea since for *every* new class you have to modify the base class. With such a design ABI stability is not possible.
Ok.
In fact, I wouldn't expect to have to perform this kind of casts,
These cast should not be needed often as I already wrote in http://lists.opensuse.org/yast-devel/2014-12/msg00025.html.
at most I would expect to do more rubist things like
tmp1.is_a?(Storage::Disk) tmp1.respond_to?(:whatever_method)
I'll do more experiments with this as soon as the prototype is easily installable. But looks pretty clear that it would be fairly easy to fix the un-rubism by adding the corresponding methods in the Ruby side in case we decide we are so purist that we cannot live with the current API. :-) So nothing to fix in the libstorage (C++) side.
Query interface ===============
The API contains methods like these:
device_graph.find_device(numeric_id) Storage::BlkDevice.find(device_graph, "/dev/sda") Storage::Filesystem.find_by_mountpoint(device_graph, "/") Storage::Filesystem.find_by_label(device_graph, "someLabel")
Questions that come to my mind.
a) Is device_graph always an object representing the whole graph?
The devicegraph always represents the whole graph.
Can I do things like this to restrict the search to a subtree?
disk1 = Storage::Disk.find(device_graph, "/dev/sda") sub_graph = disk1.to_device_graph # or maybe sub_graph = device_graph.sub_graph(disk1.sid) # or any other thing that makes sense
In theory there are many ways to get a subgraph, look at the functions in Device.h.
Storage::Filesystem.find_by_mountpoint(sub_graph, "/")
Do you have a real use-case for that? Adding subgraphs looks like a lot of work so might not be worthwhile.
No concrete example, just wondering. But the idea of restricting the search to a subtree somehow came naturally to my mind while reading. Like "look for a filesystem with this property, but only in this volume group". Probably not worth the effort, I was just curious.
b) Adding find_by_xx at demand over time doesn't look very future-proof. I would prefer to see something like Rails's #find_by, i.e.
Storage::Filesystem.find(device_graph, mountpoint: "/") Storage::Filesystem.find(device_graph, mountpoint: "/", label: "blah")
Too ruby-centric and not easy to implement in a cross-language way, I guess. But asking does not hurt. :-)
As I already wrote in http://lists.opensuse.org/yast-devel/2014-12/msg00028.html if someone takes care of bindings for a target language more things are possible.
Yes. And it should be fairly easy to implement. But I still wonder about how flexible and future-proof is to keep adding find_by_xx on demand to the API. More flexible solutions would also be more complex to use for sure. I just wonder if they are worth exploring. One obvious solution (that would need refinement, of course) if having a search object that accepts any number of key-pair filters. The most primitive form just to expose the idea would be something like (in pseudocode): s = Storage.search(device_graph, "filesystem") s.add_filter("mountpoint", "/") s.first() Or any other idea that can be implemented with a fixed set of classes and methods instead of extending the existing classes with new custom methods over time.
c) Just brainstorming. The approach in (b) could be pushed to the limit to have things like this. But actually I'm not sure if it's a good idea, just braindump.
device_graph.find(sid: numeric_id) device_graph.find(type: :filesystem, mountpoint: "/")
Lack of inline documentation ============================
Right now, the code is the documentation. I have to say that the code is very nicely structured, so it was really easy to me to browse for the methods I wanted to check (and they were all fairly small and understandable as well). But it's time to start adding documentation to the whole thing.
It's a prototype and development was halted abrupt eight months ago.
Nitpicking: boolean as parameters =================================
This is something that was already mentioned by Martin in one of the mails referred at the top. Calls like this are not 100% obvious
sda.descendants(false)
It would be nicer to have something like sda.descendants(:direct) or sda.descendants(direct: true)
Looks difficult as already mentioned in http://lists.opensuse.org/yast-devel/2014-12/msg00025.html
Ok.
[...]
Cheers. -- 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