On Fri, Feb 24, 2017 at 02:43:32AM +0100, Ancor Gonzalez Sosa wrote:
On 02/23/2017 09:31 PM, Arvin Schnell wrote:
On Thu, Feb 23, 2017 at 05:45:48PM +0100, Ancor Gonzalez Sosa wrote:
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.
For all enums there is a function to turn the value into a string. If desired the get_classname function can also be made public, although I don't see a use-case (internally it is only used at a very few places).
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.
It doesn't look like a workaround for me - but we had the discussion already a few years ago. And with strictly types enums collisions are not a problem (and they are used in libstorage-ng).
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.
Well, you know how many bug-reports we had with 'undefined method X for nilclass'? To me that shows the downside of using nil.
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.
Again, with strictly types enums that is not a problem.
ciao Arvin
--
Arvin Schnell,