Handle stringified numerics #12
Conversation
| # Puppet stringifies numerics in versions of Puppet < 4.0.0 | ||
| # Account for this in the type | ||
| begin | ||
| numeric_as_string = Integer(value) |
There was a problem hiding this comment.
@glarizza do we need to account for floats? Would it cause any harm if we were to simply s/Integer/Float/g in your PR?
There was a problem hiding this comment.
Probably wouldn't harm it - I'll make the change. Also updating the provider with mk_resource_methods instead of explicit getter/setter, so I'll push those changes and a test this morning.
|
@fpringvaldsen would you mind reviewing this? And when we hear back from Gary on the float question maybe we can add a commit and another test if he doesn't have time? |
Previously, the type would check the type of value being passed in the DSL and would use that type when setting the value in the Hocon config file (i.e. a string value would be a string in the config file, and an array would be an array value). The problem is that versions of Puppet < 4.0.0 stringify all values passed in the DSL, so it was impossible to pass a true numeric value via the DSL (since it would be stringified). This commit updates the type validation to account for this and implements a munge block to convert the value type to a numeric. A test for this case has also been added. squash
Previously, the changes made only handled Integer values and didn't account for Float values. This commit accounts for Floats and adds a test for the same.
|
Please hold off on merging this for three issues: 1.) Using Float() is going to convert integers to floating point numbers with decimal points |
The type property is a property because we need access to its value from within the `value` property's validate block. If `mk_resource_methods` is used, syncronization fails - it will constantly try to synchronize the type property. This commit changes that behavior.
|
Okay, so here's where we're at: I'm using Float() to handle floating point values, but it will convert all integer values set in the DSL to a float (i.e. the file will contain 13.0 even if 13 is set in the manifest). The tests don't check for this because 13.0 == 13 in Ruby (though 13.0.eql?(13) is false - so that's the check when/if we differentiate). We COULD add support for integer vs float in the type property to account for this, or just not care that 13.0 is entered into the file (I'd assume the former is better than the latter). |
|
Could we try to convert to an int first, and if that fails, try float, and then fall back to string? Then we wouldn't need to add the extra type? |
Previously, there was no way to determine whether a string value was intended to be an integer or a float. This commit first attempts to cast a string value as an integer. Failing that, an attempt to cast the string to a float is attempted. If that fails, and the `type` property is set to 'number', an error is raised. Tests were added to specifically test each case.
|
Modified the code and added tests. This should meet our needs. |
|
Wait, oops, forgot the munge. Please hold... |
A previous commit added detection for float/integer string values to the validation block of the `type` property. This commit adds that same logic to the munge block to the value is munged to the appropriate value
|
Okay, I noticed that the tests didn't catch my omission on the munge block, and that's because there were no tests for the type. I added some and fixed the munge. NOW we should be good... |
glarizza
force-pushed
the
fix_numeric
branch
2 times, most recently
from
41a26e6
to
e1d2ff2
Compare
| provider = described_class.new(resource) | ||
| provider.create | ||
| expect(provider.exists?).to be true | ||
| #expect(provider.value[0]).to eql(12) |
There was a problem hiding this comment.
I think we should remove this commented line.
There was a problem hiding this comment.
Done and pushed
On Thu, Apr 30, 2015 at 1:20 PM, Preben Ingvaldsen <notifications@github.com
wrote:
In spec/unit/puppet/provider/conf_setting/ruby_spec.rb
#12 (comment)
:
- it "should be able to handle a float value with number type specified" do
resource = Puppet::Type::Hocon_setting.new(common_params.merge(:setting => 'test_key_1.master', :value => 13.37, :type => 'number'))provider = described_class.new(resource)provider.createexpect(provider.exists?).to be trueexpect(provider.value[0].eql?(13.37)).to be(true)- end
- it "should be able to handle an Integer string value with number type specified" do
resource = Puppet::Type::Hocon_setting.new(common_params.merge(:setting => 'test_key_1.master', :value => '12', :type => 'number'))provider = described_class.new(resource)provider.createexpect(provider.exists?).to be true#expect(provider.value[0]).to eql(12)I think we should remove this commented line.
—
Reply to this email directly or view it on GitHub
https://github.com/puppetlabs/puppetlabs-hocon/pull/12/files#r29449776.
Gary Larizza
Professional Services Engineer
Puppet Labs
Previously, there were no spec tests to determine whether type validation and munging was successful. This commit adds them. One test is commented out because it is failing - because :array_matching is set to :all, EVERY string/array/boolean/number value comes wrapped in an array. The validation doesn't account for this currently, and a later commit will fix it.
|
@glarizza Thanks for this! I'm 👍 on this PR, everything looks good to me. The last thing I think we need to do on this is add some information on the @cprice404 or @jpinsonault could one of you give this a review? |
Previously, the type would check the type of value being passed in the DSL
and would use that type when setting the value in the Hocon config file
(i.e. a string value would be a string in the config file, and an array
would be an array value). The problem is that versions of Puppet < 4.0.0
stringify all values passed in the DSL, so it was impossible to pass a true
numeric value via the DSL (since it would be stringified).
This commit updates the type validation to account for this and implements
a munge block to convert the value type to a numeric. A test for this case
has also been added.