[yast-devel] Questions about some YCP Equivalents in ruby
Hi, While I was just fixing a bug in current head and had to test the fix anyway I assumed I could easily modify the ruby code needing to be adapted anyway to a more ruby-like style. I replaced the common usage of hash access with default parameter by YCP Code --> Ruby like replacement part["fsid"]:0 --> (part["fsid"]||0) part["type"]:`none --> (part["type"]||:none) substring(part["device"]:"",0,9) --> part["device"][0,9] Could someone with more ruby experience tell me if these replacements are reasonable? My testing showed they work, but I am not sure if there are pitfalls I am not aware of. Tschuess, Thomas Fehr -- Thomas Fehr, SuSE Linux Products GmbH, Maxfeldstr. 5, 90409 Nuernberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer, HRB 16746 (AG Nürnberg) Tel: +49-911-74053-0, Fax: +49-911-74053-482, Email: fehr@suse.de GPG public key available. -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
On 08/13/2013 01:30 PM, Thomas Fehr wrote:
Hi,
While I was just fixing a bug in current head and had to test the fix anyway I assumed I could easily modify the ruby code needing to be adapted anyway to a more ruby-like style.
I replaced the common usage of hash access with default parameter by
YCP Code --> Ruby like replacement part["fsid"]:0 --> (part["fsid"]||0) part["type"]:`none --> (part["type"]||:none)
This might cause a buggy behavior if the value was /false/ ! part["format"] = false part.fetch("format", true) --> returns false part["format"] || true --> returns true irb(main):001:0> part = {} => {} irb(main):002:0> part["format"] = false => false irb(main):003:0> part.fetch("format", true) => false irb(main):004:0> part["format"] || true => true I've found some nice article on this: http://www.dotnetguy.co.uk/post/2012/08/19/ruby-hash-fetch-returning-a-value...
substring(part["device"]:"",0,9) --> part["device"][0,9]
I haven't checked that but you can access string as it was an array, so it seems this might work well. irb(main):001:0> a = "12345678901234567890" => "12345678901234567890" irb(main):002:0> a[0,9] => "123456789" irb(main):003:0> a[0,2] => "12" irb(main):004:0> a[2,4] => "3456" Looks good.
Could someone with more ruby experience tell me if these replacements are reasonable? My testing showed they work, but I am not sure if there are pitfalls I am not aware of.
HTH Lukas -- Lukas Ocilka, Cloud & Systems Management Department SUSE LINUX s.r.o., Praha -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
On Tue, Aug 13, Lukas Ocilka wrote:
On 08/13/2013 01:30 PM, Thomas Fehr wrote:
Hi,
While I was just fixing a bug in current head and had to test the fix anyway I assumed I could easily modify the ruby code needing to be adapted anyway to a more ruby-like style.
I replaced the common usage of hash access with default parameter by
YCP Code --> Ruby like replacement part["fsid"]:0 --> (part["fsid"]||0) part["type"]:`none --> (part["type"]||:none)
This might cause a buggy behavior if the value was /false/ !
part["format"] = false part.fetch("format", true) --> returns false part["format"] || true --> returns true
I see, thanks for mentioning this. So the above construct works for every type except bool. Neverthless I rewrote it using method fetch (that so far I did not know)
I've found some nice article on this: http://www.dotnetguy.co.uk/post/2012/08/19/ruby-hash-fetch-returning-a-value...
Nice to see others did not know about fetch too ;-)
I haven't checked that but you can access string as it was an array, so it seems this might work well.
irb(main):001:0> a = "12345678901234567890" => "12345678901234567890" irb(main):002:0> a[0,9] => "123456789" irb(main):003:0> a[0,2] => "12" irb(main):004:0> a[2,4] => "3456"
Looks good.
Meanwhile I have found string method starts_with? which should be more self explanatory for this special case. Tschuess, Thomas Fehr -- Thomas Fehr, SuSE Linux Products GmbH, Maxfeldstr. 5, 90409 Nuernberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer, HRB 16746 (AG Nürnberg) Tel: +49-911-74053-0, Fax: +49-911-74053-482, Email: fehr@suse.de GPG public key available. -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
Dne 13.8.2013 14:15, Thomas Fehr napsal(a):
On Tue, Aug 13, Lukas Ocilka wrote:
part["format"] = false part.fetch("format", true) --> returns false part["format"] || true --> returns true
I see, thanks for mentioning this. So the above construct works for every type except bool. Neverthless I rewrote it using method fetch (that so far I did not know)
There are more possibilities how to handle missing keys. Hash.fetch, as mentioned above, has a disadvantage that you need to repeat the same default in every fetch() call which is error prone. The are other possibilities for setting the default: - Hash.new(<default>) - the hash returns the default when the key is missing, sets the default when creating the hash - my_hash.default = <default> - sets/updates the default, similar to the above but can be used later (for already existing hash) The disadvantage is that there is just a single default value so it cannot be used for part["fsid"]:0 part["type"]:`none replacement where you need different default values/types. However there is another possibility, the new() method takes a block as well: (see http://www.ruby-doc.org/core-2.0/Hash.html#method-c-new) a = Hash.new do |h, k| case k when "fsid" 0 when "type" :none else nil end end Then it works like this: # try the defaults a["fsid"] => 0 a["type"] => :none # fallback for unknown key a["foo"] => nil # but beware, these keys actually do not exist in the hash and are not returned # by keys() method as they are not stored there! a.keys => [] # of course, setting something overrides the defaults a["fsid"] = 42 a["type"] = :foo # read the real stored values a["fsid"] => 42 a["type"] => :foo # after storing the values above the keys are visible a.keys => ["fsid", "type"] The advantage of this solution is that the defaults are set at one place and you do not have to repeat them all over again. You just need to be careful when reading stored keys and when iterating over the hash as the defaults are not stored and thus not returned. (But that is the same as in YCP, so it should be not a problem.) You can even raise an exception when accessing an unknown key, simply change the default in the "else" branch of the case: ... else raise "unknown key #{k}" end Then e.g. a["foo"] raises "RuntimeError: unknown key foo" exception, this way you could catch some wrong usages of the hash (if the set of keys is fixed and known in advance). Anyway, in the long term we should replace the "part" hash by a native Ruby object with appropriate attributes, that would solve all the problems... -- Ladislav Slezák Appliance department / YaST Developer Lihovarská 1060/12 190 00 Prague 9 / Czech Republic tel: +420 284 028 960 lslezak@suse.com SUSE -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
Dne 15.8.2013 14:03, Ladislav Slezak napsal(a):
The are other possibilities for setting the default:
- Hash.new(<default>) - the hash returns the default when the key is missing, sets the default when creating the hash
- my_hash.default = <default> - sets/updates the default, similar to the above but can be used later (for already existing hash)
[...]
However there is another possibility, the new() method takes a block as well: (see http://www.ruby-doc.org/core-2.0/Hash.html#method-c-new)
a = Hash.new do |h, k| case k when "fsid" 0 when "type" :none else nil end end
I'm little late to reply, but I still feel that I should point out that setting a hash default is something that I consider a bad practice. The reason is that it is *unexpected* (99.99% hashes in Ruby don't set the default and just return nil for keys that are not present) and not easily *visible* in the code. To illustrate this, imagine I am debugging something and see code like this: result = my_hash["key"] Suppose that "result" is printed later in the log and it's value is non-nil. Because hash defaults are *unexpected*, it's very likely that I would assume that the "key" key was in the hash. This assumption could be wrong if a default is used and could lead me to a wrong direction. Now suppose I'd "expect the unexpected" and would want to check if a default is used. Now I'd encounter the *visibility* problem. I'd have to look where the hash is created and see if a default is set there. And that by itself is not enough, because the default could be set even at some later point, meaning any code that touches the hash is suspect. As a result, I could spend quite some time just by ensuring that the hash has no default. Because of these issues, I think it's generally better to avoid hash defaults completely. It makes the code easier to reason about, which translates to less bugs and less time spent by debugging. -- David Majda SUSE Studio developer http://susestudio.com/ -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
# dmajda@suse.cz / 2013-08-29 10:15:30 +0200:
Now suppose I'd "expect the unexpected" and would want to check if a default is used. Now I'd encounter the *visibility* problem. I'd have to look where the hash is created and see if a default is set there. And that by itself is not enough, because the default could be set even at some later point, meaning any code that touches the hash is suspect. As a result, I could spend quite some time just by ensuring that the hash has no default.
this is a great argument against reopening classes or monkeypatching, basically against ruby. ;) /me heading back under the bridge... -- roman -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
Dne 29.8.2013 14:47, Roman Neuhauser napsal(a):
# dmajda@suse.cz / 2013-08-29 10:15:30 +0200:
Now suppose I'd "expect the unexpected" and would want to check if a default is used. Now I'd encounter the *visibility* problem. I'd have to look where the hash is created and see if a default is set there. And that by itself is not enough, because the default could be set even at some later point, meaning any code that touches the hash is suspect. As a result, I could spend quite some time just by ensuring that the hash has no default.
this is a great argument against reopening classes or monkeypatching, basically against ruby. ;)
Yes, I'm aware of it :-) Nothing is black and white and one has to think about degrees of various positive and negative effects (even of the same kind) in different situations. In case of hashes with defaults, the positive effects seems small (a bit of DRY) and the negative ones large (big surprise at call site). This is amplified by the possibility to solve the problem differently (as pointed out by Ladislav in another reply to my mail). In other situations (say, RSpec adding "should" method to all objects), the situation isn't that clear (at least to me). -- David Majda SUSE Studio developer http://susestudio.com/ -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
# dmajda@suse.cz / 2013-08-29 16:10:14 +0200:
Nothing is black and white and one has to think about degrees of various positive and negative effects (even of the same kind) in different situations. In case of hashes with defaults, the positive effects seems small (a bit of DRY) and the negative ones large (big surprise at call site).
back to serious mode (on my part). you said:
To illustrate this, imagine I am debugging something and see code like this:
result = my_hash["key"]
this is a strawman, nobody sane names their variables "my_hash". so let's say we have this code: def parse input, options Parser.new(options['strict']).parse(input) end you have no idea what type `options` is, all you know (actually hope for) is it has a `[](key)` method. looking at it from the other side, how come you don't mind objects with `[](key)`? they are *not* Hash, they have all kinds of "unexpected" behaviors, so really, what's the difference? -- roman -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
On Thu, 29 Aug 2013 16:43:43 +0200 Roman Neuhauser <rneuhauser@suse.cz> wrote:
# dmajda@suse.cz / 2013-08-29 16:10:14 +0200:
Nothing is black and white and one has to think about degrees of various positive and negative effects (even of the same kind) in different situations. In case of hashes with defaults, the positive effects seems small (a bit of DRY) and the negative ones large (big surprise at call site).
back to serious mode (on my part).
you said:
To illustrate this, imagine I am debugging something and see code like this:
result = my_hash["key"]
this is a strawman, nobody sane names their variables "my_hash". so let's say we have this code:
def parse input, options Parser.new(options['strict']).parse(input) end
you have no idea what type `options` is, all you know (actually hope for) is it has a `[](key)` method.
looking at it from the other side, how come you don't mind objects with `[](key)`? they are *not* Hash, they have all kinds of "unexpected" behaviors, so really, what's the difference?
From dmajda POV problematic part is when you debug. It means you add e.g. some inspect call (e.g. in debugger) and see what happens: h = Hash.new {|h,k| k } => {} h[:a] => :a h.inspect => "{}" so this part can cause problem. If it somehow say that I am hash with defined defaults, then it is much better. Josef -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
# jreidinger@suse.cz / 2013-08-30 09:25:18 +0200:
On Thu, 29 Aug 2013 16:43:43 +0200 Roman Neuhauser <rneuhauser@suse.cz> wrote:
looking at it from the other side, how come you don't mind objects with `[](key)`? they are *not* Hash, they have all kinds of "unexpected" behaviors, so really, what's the difference?
From dmajda POV problematic part is when you debug. It means you add e.g. some inspect call (e.g. in debugger) and see what happens:
[...] ok, that makes some sense. -- roman -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
Dne 29.8.2013 16:43, Roman Neuhauser napsal(a):
so let's say we have this code:
def parse input, options Parser.new(options['strict']).parse(input) end
you have no idea what type `options` is, all you know (actually hope for) is it has a `[](key)` method.
looking at it from the other side, how come you don't mind objects with `[](key)`? they are *not* Hash, they have all kinds of "unexpected" behaviors, so really, what's the difference?
I do mind them if they behave too differently from hashes (or arrays -- depends on what kind of [] "API" the object presents). This includes behavior of [] when the key is not present. If they quack enough like a hash, I am happy, as I can mostly forget the differences and thinking about the code becomes easier. -- David Majda SUSE Studio developer http://susestudio.com/ -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
Dne 29.8.2013 10:15, David Majda napsal(a):
Because of these issues, I think it's generally better to avoid hash defaults completely. It makes the code easier to reason about, which translates to less bugs and less time spent by debugging.
I agree with that. The point is that the existing code relies on the defaults, we cannot easily get rid of them. This is legacy from YCP where defaults were enforced. And defining the defaults just once looks like a nice feature to avoid inconsistencies and mistakes. As I already wrote, the correct solution would be to replace the hash by a native Ruby object and wrap these defaults in a specific class. But I guess that won't be trivial in storage... -- Ladislav Slezák Appliance department / YaST Developer Lihovarská 1060/12 190 00 Prague 9 / Czech Republic tel: +420 284 028 960 lslezak@suse.com SUSE -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
On Tue, 13 Aug 2013 13:30:27 +0200 Thomas Fehr <fehr@suse.de> wrote:
Hi,
While I was just fixing a bug in current head and had to test the fix anyway I assumed I could easily modify the ruby code needing to be adapted anyway to a more ruby-like style.
I replaced the common usage of hash access with default parameter by
YCP Code --> Ruby like replacement part["fsid"]:0 --> (part["fsid"]||0) part["type"]:`none --> (part["type"]||:none) substring(part["device"]:"",0,9) --> part["device"][0,9]
Hi Thomas, at first I would like to mention reason why we have it in Ops, because in ycp if part is nil, then ycp return default, but ruby will raise exception. As others mention problem is false value and also nil value (if part["fsid"] have value nil, then you get 0 instead. Fetch also solve it. Or as lslezak mention nice feature with values for missing. There is one hidden problem, that ycp with `[]:` operator do implicit conversion of types, which in corner cases can cause different result for part["fsid"] = "magic"; y2milestone ("val %1", part["fsid"]:0); // ycp return nil in this case But it is usually corner cases and I personally found it quite stupid behavior. BTW1 ruby [] takes also range and it is very powerful tool in ruby, that allows various specification what elements you exactly want[1]. BTW2 for lslezak example I think it can be done better with constant so it is clear what is default like PART_DEFAULTS = { "fsid" => 0, "type" => :none } part = Hash.new { |h,k| PART_DEFAULTS[k] } NOTE last but not least be aware that in ruby everything is passed as reference, even constants. So you don't get copy there. If you want shallow copy then call .dup on specified object ( immutable object doesn't have it or raise exception ). For deep copy call deep_copy method, then copy everything except strings ( string are exception, because it is used a lot in old ycp code and it cause significant slowdown in ruby ( and old code always use builtins to modify string ), so maybe we need to introduce new deep_copy for new code, that copy also strings, but usually you don't need deep copy ). Josef [1] http://ruby-doc.org/core-2.0/Array.html#method-i-5B-5D -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
Dne 16.8.2013 12:31, Josef Reidinger napsal(a):
BTW2 for lslezak example I think it can be done better with constant so it is clear what is default like
PART_DEFAULTS = { "fsid" => 0, "type" => :none }
part = Hash.new { |h,k| PART_DEFAULTS[k] }
Yeah, thanks, this looks much better! -- Ladislav Slezák Appliance department / YaST Developer Lihovarská 1060/12 190 00 Prague 9 / Czech Republic tel: +420 284 028 960 lslezak@suse.com SUSE -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
participants (6)
-
David Majda -
Josef Reidinger -
Ladislav Slezak -
Lukas Ocilka -
Roman Neuhauser -
Thomas Fehr