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: [email protected]
To contact the owner, e-mail: [email protected]
