Add method to safely get or check the existence of attributes

[mcquin]

Resolves #794 by extending the attribute? and get_attribute DSL methods to apply to sub-attributes. Attributes can be specified as a /-delimited string, a list of string keys, a list of symbol keys, or a combination thereof.

Open to suggestions like:

1. Only supporting a list of strings and symbols (get rid of that /-delimiter stuff, that's too fancy)
2. Adding similar support to set_attribute in this PR (was gonna do it separate since the implementation is different).
3. ___________________________________________________________________________.

[thommay]

🎉 👏 👍

[mcquin]

@thommay minor change to require at least one parameter (o/w attribute? returns true and get_attribute returns all attributes and that would just be not good).

[tas50]

+1

[mcquin]

Thoughts from @chef/client-core ? :)

[thommay]

@mcquin good catch!

[jaymzh]

Hmm. Should we instead try to make this match with the way we did node.rm? (which is node.rm('foo', 'bar')? i.e, take a list? What happens if I have an attribute with a slash in it (which is common in, say, the filesystem plugin).

I like the idea... but I don't dig the implementation.

[mcquin]

@jaymzh whoa attributes with / characters in them is a real good reason to not support that! I'll rip that out. It currently does support lists of strings and symbols. :)

[jaymzh]

Yeah, sorry, it's very early, I only read the description, and hadn't gotten to the code. Which I've now read. Minus the / bit, I'm good. Looks great. 👍 from me once that's changed.

[mcquin]

Updated to remove support for / @jaymzh

[lamont-granquist]

array indexes?

[mcquin]

Arrays don't respond to has_key?.

[lamont-granquist]

copied and adapted from some other code of mine (still a bit Before Caffeine for me so I may be overlooking something):

begin
  keys.inject(@data) { |memo, key| memo[key] }
rescue NoMethodError
  nil
end

[mcquin]

Ahh that is so nice. It breaks in the case where some intermediate key is not a hash, though. Hm...

[mcquin]

Haha rescue NoMethodError, TypeError works

[thommay]

👍

[coderanger]

There is an RFC declaring . is the One True Separator, if we want to support that (though an array should always be supported too for places where that character is in a key).

[mcquin]

@coderanger I thought about that. But I didn't want to mix provides "foo/bar" and attribute?("foo.bar").

[coderanger]

Sure, but the former should be fixed, not the latter :)

[mcquin]

🚧

[lamont-granquist]

I think its time to argue about RFC 18 tomorrow AM:

chef-boneyard/chef-rfc#195

[coderanger]

To be clear, I'm still 👍 on this as written. Just if we did want to support the single-string syntax it should be with .. This seems like an advanced-mode kind of API though, so I don't think we need to put the max level of UX slickness on it.

[jaymzh]

👍

[tas50]

@chef/client-core can we get another set of eyes on this since there's been some changes since the initial 👍 s

[lamont-granquist]

|| attrs.is_a?(Array)?

[mcquin]

I don't think we want to allow Array access here. The idea is to prevent accessing values as subattribute keys. We expect the structure to be a Mash until we reach a value.

[lamont-granquist]

Yeah but Mashes turn into Arrays once you traverse them, and attrs[key] is fine to run on an Array.

I think if you set it up and try to do something like

safe_get_attributes("etc", "passwd", "group", "wheel", "members", 0)

It'll throw a TypeError instead of giving you back "root" (on my Mac anyway).

[mcquin]

Mashes turn into Arrays once you traverse them

Whoa, ugh. Ok.

[thommay]

👍