-
-
Notifications
You must be signed in to change notification settings - Fork 46
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
Add :array_append parameter to shellvar type/provider #62
Conversation
should_arr = Array(should) | ||
|
||
# Join and split to ensure all elements are parsed | ||
is_str = is.is_a?(Array) ? is.join(' ') : is |
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.
This join/split seems unnecessary, could you just do:
is_arr = is.is_a?(Array) ? is : is.split(/\s+/)
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.
Actually, it seems is
is always cast as an array, but it might contain either multiple values, or a string of joint values (i.e. ['a', 'b']
or ['a b']
), hence this code.
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.
Yeah, different behaviour for array values or not as we're using array_matching. I'm just concerned that this might not work correctly with actual arrays rather than strings containing spaces. Maybe use is_array?
from the provider or something to determine it exactly?
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.
Hmmm. is_array?
determines if a path is an array or not. Here, the problem is turning the value into an array, splitting it by values. The fact of using array_append
should force considering the value as an array, no?
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.
Yes, but is_array?
might then help you know whether to split the value or not.. without forcing it to join and split again. If an array had whitespace in it, this might split it incorrectly? If is_array?
is true, then we can skip joining/splitting.
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.
That's if there is already a value in the tree. Otherwise, there's no way to tell.
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.
Of course.. but in that case is
won't be an array.
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.
Hehe, good point ;-)
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.
Sometimes, the existing value is a string (see the STR_LIST
examples in the fixtures), so is_array?
will return false
, but it should still be treated as an array, and split to compare the values…
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.
Ah, string array values, fair enough!
👍 squash and push |
See http://serverfault.com/a/557032/29328 for original idea.
appending to arrays
will change
GRUB_CMDLINE_LINUX="quiet splash"
toGRUB_CMDLINE_LINUX="quiet splash cgroup_enable=memory"
.will also change
GRUB_CMDLINE_LINUX="quiet splash"
toGRUB_CMDLINE_LINUX="quiet splash cgroup_enable=memory"
.