On Wed, Oct 07, 2015 at 02:12:44PM +0200, Ancor Gonzalez Sosa wrote:
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.
I wanted to have an overview of the classes in the library, so I ended up fixing this aspect: https://github.com/aschnell/libstorage-bgl-eval/pull/2 (BTW I am missing a ReadTheDocs hosting site for C++/Doxygen) But I still have questions: What is a Holder? It seems to be characterized by having a source sid and a target sid, which still does not give me a hint. Its subclasses Subdevice and User are even more opaque to me. Environment sounds like a candidate for a kitchen sink of unrelated hacks. Can we describe it with the purpose of limiting its scope? Devicegraph should be renamed to DeviceGraph. It just isn't a single word. Why is there Storage::copy_devicegraph if Devicegraph is_a noncopyable? The "Encryption" name is bothering me because encryption is a concept whereas all the other subclasses of Device are things. storage::Disk::get_transport returns a storage_legacy::Transport. A "modern" object returning a "legacy" object does not feel right, but I don't know enough to suggest an alternative. -- Martin Vidner, YaST Team http://en.opensuse.org/User:Mvidner Kuracke oddeleni v restauraci je jako fekalni oddeleni v bazenu