On 02/23/2017 09:31 PM, Arvin Schnell wrote:
On Thu, Feb 23, 2017 at 05:45:48PM +0100, Ancor Gonzalez Sosa wrote:
My Hack Week project is in that state in which all the important parts work and now it's time to decide if it's worth investing a couple of extra days to finish it all the way down.
Since the ultimate goal is to change how YaST interacts with storage-ng, I would need the opinion of as many YaST developers as possible.
So please, please, please, take a look to this and give your opinion. https://github.com/yast/yast-storage-ng/pull/169
Some examples using the swig bindings directly can be better,
Thanks for the corrections.
e.g.:
puts "Ptable type #{a_storage_msdos_ptable.type}" #=> Prints "Ptable type 1"
puts "Ptable type #{a_storage_msdos_ptable.to_s}"
Ok, so let's use another example. puts "Mount by #{blk_fs.mount_by}" #=> Prints "Mount by 2" This time, we already used Filesystem#to_s to display the filesystem type. So we cannot apply the same workaround to this other enum-typed field in the same class. As I wrote in the PR description, for some cases we have workarounds and for others don't. Abusing the #to_s method of Filesystem or PartitionTable to get the readable version of its type is just a workaround for one of the many drawbacks explained for whole enums approach. It does not address the root issues.
or
puts "extended" if [Storage::FsType_EXT2, Storage::FsType_EXT3, Storage::FsType_EXT4].include?(a_storage_filesystem)
puts "extended" if Storage.ext?(a_storage_filesystem)
Ok, ext? is new (part of my hackweed project). So far ext2 and ext3 were not implemented.
ptable.type == Storage::PtType_GPT
Storage.gpt?(ptable)
Once again, all those methods mixed altogether directly on the Storage namespace instead of being the responsibility of more focused classes look like a workaround to me. Not to mention the risk of name collision if all the values of all the possible enums need to have a corresponding method in the same place.
Concerning the nil instead of exceptions:
a_y2storage_unused_partition.filesystem #=> nil
What happens here (nilclass exception?)?
a_y2storage_unused_partition.filesystem.mount_by?
If you call NilClass#mount_by? you will get an exception, of course. Not sure what's the point of the question, though.
I also thought about placing some classes in sub-namespaces but mainly to resolve the upcoming name collision of DASD (the "disk") and DASD (the partition table).
In addition to the class names, this serves as a nice example of my point above. If at some point we have DASD as a possible value for a PartitionType enum and as a possible value for a, let's say, Transport or DiskType enum. Will the Storage.dasd? method be reused for both? Quite tricky.
The most important question is - do you prefer the proposed approach and API* or do you prefer to continue accessing directly to libstorage-ng from YaST?
For me it will make programming in Ruby more difficult since I have to know two APIs (already mentioned in doc/design-decisions.md).
That's a valid point that affects those who know/develop the libstorage API and, in addition, has to develop Ruby parts of YaST (i.e. the S subteam). In my case, I'm fine with paying that price. Sticking to the libstorage API also has a price since it's already forcing us to use refinements, monkey-patching, EnumMapping, custom RSpec matchers[1], etc. I would gladly pay the price of learning two (very similar) APIs in exchange for getting rid of all that. On the other hand, most of the YaST Ruby code (now and in the future) should be written by other Ruby developers (the S-subteam should be short-lived[2]). All that people will only have to learn one API. Thinking in the long term, I would prefer that API to follow Ruby conventions and to not have the problems described in the PR (like typed collections, need of downcasting, etc). Cheers. [1] On one hand, we have defined custom RSpec matchers to be able to deal with the "un-rubyness" of objects coming from libstorage. On the other hand, we cannot use some of the most common RSpec matchers in situations with some libstorage object involved, because the type checking imposed by Swig blows the matchers away. [2] I expect the subteam itself to be relatively short-lived, not the members (I hope we will all survive to storage-ng). :-) -- 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