-
Notifications
You must be signed in to change notification settings - Fork 580
Loosen the restrictions of upcase and allow for recursion of the objects... #419
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.
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
Conversation
Hey @elyscape, thanks for your feedback on the latest upcase enhancement, I know there was talk about why we needed it. The reason is so that we can get the keys however the user passes in the object whether it is 'grant', 'Grant' or 'GRANT' or any type of permutation. Hope this captures some of your thoughts. Also I had some of my own, like do we care what kind of object it is if it responds to upcase, symbols don't in ruby 1.8.7 but they do in 1.9.3 and 2.x |
This will fail if you pass in a hash that contains a numeric type anywhere inside it. I think the best thing to do would be to validate that the initial object passed in as either an Array, a Hash, or something that responds to |
@@ -21,18 +21,18 @@ module Puppet::Parser::Functions | |||
|
|||
value = arguments[0] | |||
|
|||
unless value.is_a?(Array) || value.is_a?(String) || value.is_a?(Hash) | |||
unless value.is_a?(Array) || value.is_a?(Hash) || value.respond_to?(:upcase) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the spacing changes here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IDE, generated white spaces
As a side note, why are we checking that the number of arguments passed in is at least 1? Shouldn't we be checking that it's exactly 1, considering we only use the first argument? |
I agree we should check to see if we are being passed at least one argument, didn't notice it was missing. |
No, we are checking to see if we're being passed at least one argument, but I'm asking why we're not instead checking to see if we're being passed exactly one argument. That is, where we currently have: raise(Puppet::ParseError, "upcase(): Wrong number of arguments " +
"given (#{arguments.size} for 1)") if arguments.size < 1 it would make more sense if we had: raise(Puppet::ParseError, "upcase(): Wrong number of arguments " +
"given (#{arguments.size} for 1)") if arguments.size != 1 |
Instead of this check we use arity and ensure that we are only ever passed one parameter. This PR #249 actual does this for all of the functions but waiting for Puppet 4 to merge and slate for Stdlib 5 |
521339f
to
9efe194
Compare
raise(Puppet::ParseError, 'upcase(): Requires an ' + | ||
'array, string or hash to work with') | ||
'array, hash or object that responds to upcase in order to work') | ||
end | ||
|
||
if value.is_a?(Array) | ||
# Numbers in Puppet are often string-encoded which is troublesome ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code to deal with the fact that numbers are often string-encoded seems to have been removed? Will this work with numbers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as it is passed as a String type it will work as it always has, that being said, with future parser it will fail if it is passed a Numeric type, but this was always going to be the case.
…cts and only worry if the object responds to upcase
@mhaskel docs added. |
Loosen the restrictions of upcase and allow for recursion of the objects...
... and only worry if the object responds to upcase