Once upon a time... =================== As part of the PBI titles "Evaluate design of future-proof storage subsystem", I took a look to the content of https://github.com/aschnell/libstorage-bgl-eval/ This mail can be considered to some extend a follow up of previous evaluations and discussions in this mailing list. Most feedback received from those threads has been already taken into account into the current prototype. Thus, some topics discussed there are outdated but some information is still useful. http://lists.opensuse.org/yast-devel/2014-10/msg00045.html http://lists.opensuse.org/yast-devel/2014-12/msg00016.html First impression ================ First thing I noticed in that repository is the lack of information about building and installing. In every github's README I expect to find: (1) how to build the thing, (2) how to compile the inline docs or a link to an up-to-date compiled version, (3) in (open)SUSE related stuff I also expect instructions to download or build a rpm. That being said, I took a look to the Ruby examples and testsuite and in general I liked what I saw a lot. So first of all, congrats for the great work! 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 In fact, I wouldn't expect to have to perform this kind of casts, at most I would expect to do more rubist things like tmp1.is_a?(Storage::Disk) tmp1.respond_to?(:whatever_method) 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? 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 Storage::Filesystem.find_by_mountpoint(sub_graph, "/") 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. :-) 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. 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) Nickpicking: 42 =============== I found the usage of 42 and 43 for the sids in examples and tests a little bit confusing. I grepped the source code and found that we simply use autoincremental integer to assign the sids starting with 42. Nothing against it, but please make it more obvious (and robust, in case we decide 42 is not cool anymore) in the testsuites. :-) Epilogue ======== That's all folks! -- 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