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

On Fri, 24 Feb 2017 08:26:30 +0100 Arvin Schnell wrote:

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)

To be honest I do not like both. I like to have ptable.type.gpt? or one level ptable.gpt?

...

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.

What is difference between specified exception and nil class exception? in both case it is programmer error and need to be fixed. If we do not want exception, it can be possible to Null object pattern and define behavior for non-existing filesystem, but

...

...

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.

As I write above, populating Storage namespace looks wrong. Enums on other hand is a bit previous century technique and used only very rare in ruby and looks a bit strange. So for both case I prefer to have disk.dasd? or disk.type.dasd? ( or what is sometimes used `disk.type == :dasd` but I think it is more error prone) and `disk.partitions.any?(&:dasd?)`

ciao Arvin

Just my 2c Josef -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org

On Fri, 24 Feb 2017 10:55:25 +0100 Arvin Schnell wrote:

On Fri, Feb 24, 2017 at 09:28:24AM +0100, Josef Reidinger wrote:

...

What is difference between specified exception and nil class exception? in both case it is programmer error and need to be fixed. If we do not want exception, it can be possible to Null object pattern and define behavior for non-existing filesystem, but

We also had that discussion already. Even in Wikipedia the Null Object pattern gets strong criticism.

Link provided - https://en.wikipedia.org/wiki/Null_Object_pattern#Criticism I agree with criticism there that it should not replace check for nil just for better readability. But in this case it is valid storage setup and I think it is possible to find reasonable behavior in such case ( like user friendly to_s method or raise exception if needed ). Second half of criticism I think not affect ruby and storage-ng. Let see how it will work in reality in bootloader code with storage-ng. now: def with_btrfs?(partition) partition.filesystem.type == ::Storage::FsType_BTRFS rescue Storage::WrongNumberOfChildren, Storage::DeviceHasWrongType # No filesystem in the partition false end can be: def with_btrfs?(partition) partition.filesystem.type.btrfs? end when there is not filesystem in partition, we have NoneFilesystem with NoneType that obviously return false for btrfs? another example from current yast-storage-ng now: begin partition_table = @disk.partition_table partition_table.partitions.each do |partition| ret << partition.table_row(FIELDS) end rescue Storage::WrongNumberOfChildren, Storage::DeviceHasWrongType end Can be without this begin and rescue as for not existing partition table there is no partitions. I was curious how often this pattern can be seen in yast-storage-ng so I try grep -R "rescue.*Storage::DeviceHasWrongType" * | wc -l and result is 33 So it is repeated 33 times. That code really "smells" for me. Josef -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org

On Fri, 24 Feb 2017 14:02:46 +0100 Arvin Schnell wrote:

On Fri, Feb 24, 2017 at 01:51:42PM +0100, Josef Reidinger wrote:

...

Let see how it will work in reality in bootloader code with storage-ng.

now:

def with_btrfs?(partition) partition.filesystem.type == ::Storage::FsType_BTRFS rescue Storage::WrongNumberOfChildren, Storage::DeviceHasWrongType # No filesystem in the partition false end

can be:

def with_btrfs?(partition) partition.filesystem.type.btrfs? end

Or can be:

def with_btrfs?(partition) partition.has_filesystem && is_btrfs(partition.filesystem) end

No exceptions, no NullObject, no enum, no type classes. For me straightforward readable code.

As I already wrote: The style is up to you.

ciao Arvin

Just question. Where is defined method is_btrfs? It is in ruby global namespace? or Yast namespace? I am not aware that such method exists. Josef -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org

On 02/24/2017 02:43 AM, 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:

...

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

So let's me try to redirect this thread before it becomes a re-edition of the exceptions vs nil one. Don't let one or two single controversial points ruin the full discussion[*]. My proposal is to avoid exposing the libstorage API all along YaST because I consider that exposure to be negative in the long run. Let's focus only in the less controversial points of that reasoning. - Constantly performing downcasts in the Ruby code. I believe we can agree that's the less Rubyist thing ever. - Exposing (SWIG/C++) artifacts, like strictly-typed vectors instead of Ruby-style collections, all along YaST. - Quite hard to add YaST-oriented logic on top. At this point there is already quite some extra logic we have needed to add, using/inventing some workarounds to make that possible (refinements, monkey-patching and more). - Workarounds needed when using standard Ruby tools like RSpec, caused by the fact that we don't do things in the standard Ruby way. I'm not discussing whether duck-typing is better than strict type-checking or whether enums and exceptions are better mechanisms than classes and nil. I'm just saying than doing things in a different way that the whole Ruby ecosystem is constantly causing extra work on our side[*]. - Mixing bytes (Fixnum) with Y2Storage::DiskSize objects.

...

...

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).

So far, this is the only clear argument raised against adding a Ruby layer between libstorage-ng-ruby and the rest of YaST. The rest of the thread has "focused" on discussing one or two particular points of the initially proposed API[*]. So I want to leave those controversial points out of the discussion and focus again in the main question. THE_ULTIMATE_QUESTION << Is it worth introducing that extra layer in order to fix the above-mentioned problems (that we have been suffering for some time) at the cost of forcing the S subteam members to live with two versions of the API in their heads? EOF_ULTIMATE_QUESTION Again, this is a question for the whole YaST team. So please, don't get scared by the amount of mails in the thread discussing small API design details and give your opinion on the main topic. Cheers. [*] All those discussions (nil vs exceptions, enums vs classes) has already proved to be useless in the past regarding the goal of reaching consensus, so we should stop them. But IMHO they have proved something. That there are two clearly defined sides with two clearly defined opinions: developers with a Ruby background vs developers with a C++ background. We will never reach a full agreement on which approaches are better for each controversial point. But using the Ruby approach in the Ruby code and the C++ approach in the C++ one looks to me like the only way to avoid discussing the same over and over. -- 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

It took me until just now to read that entire discussion. On 24.02.2017 16:51, Ancor Gonzalez Sosa wrote:

THE_ULTIMATE_QUESTION <<

Is it worth introducing that extra layer in order to fix the above-mentioned problems (that we have been suffering for some time) at the cost of forcing the S subteam members to live with two versions of the API in their heads?

EOF_ULTIMATE_QUESTION

Yes, it is worth it. I like the approach. It's even harder if you keep moving between all the different worlds we have (and that we will have to keep and maintain until end of life for SLE-12 which is far away): - The yast-storage-ng Ruby world - The libstorage-ng C++ world - The legacy yast-storage Ruby (converted from YCP) world - The legacy libstorage C++ world Changing back and forth between them (as I constantly do) keeps confusing the hell out of me, so I welcome every little bit of more consistency. Agreed, Ancor's approach introduces yet another API that is only loosely connected to the libstorage-ng C++ API. That is a downside, agreed. But then, we already have a lot of those things all over the place: - member variable assignment and direct use in Ruby rather than the C++ getters and setters: obj.var = 42 rather than obj.set_var( 42 ) obj.var rather than obj.get_var() - automagical search and replace for C++ predicates obj.foo? rather than obj.is_foo() - Ruby refinements at certain places - Convenience classes in Ruby on top of the libstorage C++ API that have no counterpart there So, for me it's completely separate worlds already, so we might as well go all the way and finally get it consistent. That would also add the benefit that we know where to look for documentation since the new Ruby wrapper classes hopefully come with it. Kind regards -- Stefan Hundhammer YaST Developer SUSE Linux GmbH GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg) Maxfeldstr. 5, 90409 Nürnberg, Germany -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org

On Fri, Feb 24, 2017 at 10:37:24AM +0100, Josef Reidinger wrote:

Nil instead of exceptions -------------------------

make sense for me, another option that make sense for me is NullObject, that allows some introspection and e.g. better writting to log.

Exceptions come with introspection and logging. ciao Arvin -- Arvin Schnell, Senior Software Engineer, Research & Development SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) Maxfeldstraße 5 90409 Nürnberg Germany -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org

On Fri, Feb 24, 2017 at 01:34:31PM +0100, Josef Reidinger wrote:

On Fri, 24 Feb 2017 10:58:19 +0100 Arvin Schnell wrote:

...

On Fri, Feb 24, 2017 at 10:37:24AM +0100, Josef Reidinger wrote:

...

Nil instead of exceptions -------------------------

make sense for me, another option that make sense for me is NullObject, that allows some introspection and e.g. better writting to log.

Exceptions come with introspection and logging.

ciao Arvin

We also discussed it in past and now with seeing that I need to rescue around every call for valid storage setup in bootloader, I found it even more annoying.

You don't have to. E.g. instead of calling 'partition.get_filesystem()' and catching an exception you can also query 'partition.has_filesystem()' beforehand. The programming style is up to you. ciao Arvin -- Arvin Schnell, Senior Software Engineer, Research & Development SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) Maxfeldstraße 5 90409 Nürnberg Germany -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org

On 02/24/2017 01:57 PM, Josef Reidinger wrote:

On Fri, 24 Feb 2017 13:51:42 +0100 Arvin Schnell wrote:

...

On Fri, Feb 24, 2017 at 01:34:31PM +0100, Josef Reidinger wrote:

...

On Fri, 24 Feb 2017 10:58:19 +0100 Arvin Schnell wrote:

...

On Fri, Feb 24, 2017 at 10:37:24AM +0100, Josef Reidinger wrote:

...

Nil instead of exceptions -------------------------

make sense for me, another option that make sense for me is NullObject, that allows some introspection and e.g. better writting to log.

Exceptions come with introspection and logging.

ciao Arvin

We also discussed it in past and now with seeing that I need to rescue around every call for valid storage setup in bootloader, I found it even more annoying.

You don't have to. E.g. instead of calling 'partition.get_filesystem()' and catching an exception you can also query 'partition.has_filesystem()' beforehand. The programming style is up to you.

ciao Arvin

OK, I do not know about it.

Ancor what was reason to not use this kind in bootloader and yast-storage-ng instead of bunch of rescues?

The reason is that some of those methods (like has_partition_table) were not there initially, they were added on demand. Defining an extra method for every relationship we want to have in our model is just duplicating the existing and standard Ruby mechanism for that (returning nil). Assume there is always a "has_xxx" method to do the check in advance. There are still many situations in which that is way less practical than returning nil. For example, something as simple as this with the typical Ruby behavior (returning nil) def compare_fs(partition1, partition2) partition1.filesystem == partition2.filesystem end With the current approach of rescuing exceptions or using the check method in advance (assuming that particular check method exist). def compare_fs(partition1, partition2) if partition1.has_filesystem if partition2.has_filesystem partition1.filesystem == partition2.filesystem else false end else !partition2.has_filesystem end end The same problem of checking the value of several attributes can be generalized. Something like this would be quite common in Ruby (and is, in fact, the principle of many RSpec matchers that we cannot currently use). def equivalent?(object1, object2, attributes = []) attributes.each do |attr| return false if object1.send(attr) != object2.send(attr) end true end That will crash very easily and most ruby developers wouldn't have expected because disk.partition_table or partition.filesystem just look like attributes. The behavior of the methods when returning a value that is not set should be uniform. If checking for a not-set integer or string typically returns nil, checking for a not-set filesystem should do the same. It must not be different and raise a very particular kind of exception just for some returned types. Cheers. -- 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

On 02/24/2017 10:37 AM, Josef Reidinger wrote:

On Thu, 23 Feb 2017 17:45:48 +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

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?

Thanks

* In general, minor details in the API can be ironed out in the short future.

let me comment it point by point:

Arrays instead of strictly-typed vectors ----------------------------------------

I think it make sense to have a bit specialized arrays, but it do not make sense to not act as array, so for me the best way would be to have something like `class MountPoints < Array` which have special filters like mountpoints.snapshots which return again MountPoints, but only btrfs snapshots. This allow easy extending in future and still it act like array.

We have DevicesList and all its subclasses for that (already presented in a mail some time ago, usually referred as "query API" and extensively used in yast2-bootloader, for example). But since that was already provided by the current yast2-storage-ng, I wanted to leave it out of the discussion. With this new API, DevicesList & friends would still be there but MUCH, MUCH better integrated (no more tricks or refinements). For example, you could do things like dvcegraph.all_filesystems.with_mountpoint("/boot") disk.all_partitions.with {|p| p.size > 10.GiB && p.boot? }.map(&:name) dvcegraph.all_disks.with(name: /vd/).all_partitions.with(number: 1)

No more downcasting -------------------

good improvement

Nil instead of exceptions -------------------------

make sense for me, another option that make sense for me is NullObject, that allows some introspection and e.g. better writting to log.

Classes instead of constants and enums -------------------------------------

nice improvement

All sizes as DiskSize objects -----------------------------

makes sense as it is not only integer, but have its own logic

Adding methods needed by YaST -----------------------------

makes sense

Less surprising API (for Rubyists) ----------------------------------

nice just one more idea. I really hate to type everywhere staging graph. So why not have `def all(graph = Storage.staging)` as I think majority of other Yast modules are interested mainly in staging graph.

Well, being picky it would be def all(graph = StorageManager.instance.staging) but ok. :-)

Overall I like it.

Josef

Cheers. -- 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