Use a more expressive method of rewriting values #975
Conversation
ekohl
force-pushed
the
rewrite-to-more-expressive-version
branch
from
5c5fb15 to
d2a0078
Compare
| value[k] = v.to_i if v =~ %r{\A[-+]?[0-9]+\z} | ||
| end | ||
| value | ||
| value.transform_values { |v| v.match?(%r{\A[-+]?[0-9]+\z}) ? v.to_i : v } |
There was a problem hiding this comment.
Or is this better
| value.transform_values { |v| v.match?(%r{\A[-+]?[0-9]+\z}) ? v.to_i : v } | |
| value.transform_values do |v| | |
| if v.match?(%r{\A[-+]?[0-9]+\z}) | |
| v.to_i | |
| else | |
| v | |
| end | |
| end |
There was a problem hiding this comment.
Personally I think that’s a little more readable than the terser syntax but not a strong preference, and don’t claim to be a Ruby expert.
ekohl
force-pushed
the
rewrite-to-more-expressive-version
branch
2 times, most recently
from
89b508d to
68d312b
Compare
| @@ -106,9 +106,6 @@ def validate_value(value) | |||
| def munge_value(value) | |||
| return value if value(:autoconvert) == :false | |||
|
|
|||
| value.each do |k, v| | |||
| value[k] = v.to_i if v =~ %r{\A[-+]?[0-9]+\z} | |||
There was a problem hiding this comment.
This would fail on Ruby 3.2 where [] =~ // is no longer defined, breaking Puppet 8. That's why I switched over to the string check. I also use match? because it's faster if you don't need the resulting match.
There was a problem hiding this comment.
but now
Failure/Error: expect(parameter[:value]).to eq(value)
expected: {"message-ttl"=>"1800000"}
got: {"message-ttl"=>1800000}
as said I've add next if array on the value iteration and ti "fix" the test on puppet 8
There was a problem hiding this comment.
expected: {"message-ttl"=>"1800000"}
got: {"message-ttl"=>1800000}
That's a side effect of the change. Previously the passed in value was modified in place and the tests (incorrectly) relied on that. I've modified that.
as said I've add next if array on the value iteration and ti "fix" the test on puppet 8
I think that's wrong. Your code can still fail on other non-string and non-array types because https://bugs.ruby-lang.org/issues/15231 changed the behavior.
[1] pry(main)> 123 =~ /regex/
NoMethodError: undefined method `=~' for 123:Integer
from (pry):1:in `__pry__'
[2] pry(main)> {} =~ /regex/
NoMethodError: undefined method `=~' for {}:Hash
from (pry):2:in `__pry__'
That's why I explicitly chose is_a?(String).
You could write /regex/.match?(v) but you'd get a TypeError if you pass in a non-string:
[3] pry(main)> /1234/.match?('123')
=> false
[4] pry(main)> /1234/.match?([])
TypeError: no implicit conversion of Array into String
from (pry):4:in `match?'
9044b33 to
16373fc
Compare
| parameter[:value] = value | ||
| expect(parameter[:value]).to eq(value) | ||
| parameter[:value] = { 'message-ttl' => '1800000' } | ||
| expect(parameter[:value]).to eq({ 'message-ttl' => 1_800_000 }) |
There was a problem hiding this comment.
this will get turned back into a string if needed, I assume?
There was a problem hiding this comment.
As far as I know, the code always did this. The tests just weren't explicit about it and relied on the side effect of munge also changing the original value. Now it's explicit about it. I don't have Ruby 2.7 at hand, but I'd expect this test change to also pass with the previous code.
This achieves almost the same, except that this version doesn't change the original value. If that's really desired, `transform_values!` can be used. The tests are modified to stop counting on the value being replaced by explicitly writing out the expected value. It also switches to a string check and `match?` because in modern Ruby `=~` is no longer defined for arrays. This is needed for Puppet 8 compatibility.
ekohl
force-pushed
the
rewrite-to-more-expressive-version
branch
from
16373fc to
3e4d3c3
Compare
This achieves almost the same, except that this version doesn't change the original value. If that's really desired,
transform_values!can be used. It makes it clearer what's going on.