Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use a more expressive method of rewriting values #975

Merged
merged 1 commit into from
Apr 3, 2024
Merged

Use a more expressive method of rewriting values #975

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions lib/puppet/type/rabbitmq_parameter.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,9 +106,12 @@ 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}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?'

value.transform_values do |v|
if v.is_a?(String) && v.match?(%r{\A[-+]?[0-9]+\z})
v.to_i
else
v
end
end
value
end
end
5 changes: 2 additions & 3 deletions spec/unit/puppet/type/rabbitmq_parameter_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,8 @@
end

it 'accepts a valid hash for value' do
value = { 'message-ttl' => '1800000' }
parameter[:value] = value
expect(parameter[:value]).to eq(value)
parameter[:value] = { 'message-ttl' => '1800000' }
expect(parameter[:value]).to eq({ 'message-ttl' => 1_800_000 })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will get turned back into a string if needed, I assume?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

end

it 'does not accept an empty string for definition' do
Expand Down
Loading