default(:option) { value } doesn't respect << when value is Array

[mcquin]

# lib/ohai/config
require 'mixlib/config'
module Ohai
  class Config
    extend Mixlib::Config

    default(:plugin_path) { default_plugin_path }

    private
    def self.default_plugin_path
      [ File.expand_path(File.join(File.dirname(__FILE__), 'plugins')) ]
    end
  end
end

claire> bundle exec pry
[1] pry(main)> require_relative "lib/ohai/config"
=> true
[2] pry(main)> Ohai::Config[:plugin_path]
=> ["/Users/clairemcquin/oc/ohai/lib/ohai/plugins"]
[3] pry(main)> Ohai::Config[:plugin_path] << "plugiiiinz"
=> ["/Users/clairemcquin/oc/ohai/lib/ohai/plugins", "plugiiiinz"]
[4] pry(main)> Ohai::Config[:plugin_path]
=> ["/Users/clairemcquin/oc/ohai/lib/ohai/plugins"]
[5] pry(main)> Ohai::Config[:plugin_path] += ["plugiiiinz"]
=> ["/Users/clairemcquin/oc/ohai/lib/ohai/plugins", "plugiiiinz"]
[6] pry(main)> Ohai::Config[:plugin_path]
=> ["/Users/clairemcquin/oc/ohai/lib/ohai/plugins", "plugiiiinz"]
[7] pry(main)> quit
claire> bundle show mixlib-config
/opt/chefdk/embedded/lib/ruby/gems/2.1.0/gems/mixlib-config-2.2.1

[mcquin]

@danielsdeleo @jkeiser I could use a little help on this.

Default options with blocks are designed to be re-evaluated until the value is set. Using <<, instead of +=, doesn't trip this setter method, so the option isn't considered to have been set.

Evaluating the block the first time we see it would solve the append problem, but would break re-evaluating the default value. I've thought about overriding the :<< method if the value of the block responds to it, so that the internal set function will be called. I think that would have to be defined after evaluating the default block, but my metaprogramming skills aren't strong.

[jkeiser]

I don't think you can both recalculate the value every time and support append. Like, if the user appends 'x', and then the next time we re-evaluate, should the value have 'x' in it or not? What if the re-evaluate returns something different than last time? Also, note that this isn't limited to <<. If you do that, you need to support .push, .pop, .shift, .unshift, .append ... the slippery slope argument usually sucks, but I think it applies here.

What you're observing is that it's surprising that you get a value back from a property, and it lets you modify that value, and the next time you get the property value, your modification didn't stick. There are three things you can do here:

1. Honor the modification.
   • This can be accomplished by storing the value after first access.
2. Don't let the user modify the value.
   • This can be accomplished by calling .freeze on the value. Any attempt at modification will fail.
3. Make it not surprising somehow.
   • I don't know how you could do this; there would need to be a way of communicating it just by the function name. But, there are functions out there that return writeable values and constantly recalculate them, so it may be possible.

If we want to keep the semantic of default values being computed, we can do #2. If we want to flip it around, we do #1. 3 might not be possible.

There are a couple of "middle ways" to get around this:

1. Create a second keyword, mutable_default, or some such, which caches the value on first access so that it can be modified. Freeze all values returned from default.
2. Store mutable (non-frozen) values on the instance in case they get mutated; recompute non-frozen values. Defaults can indicate which they are by returning frozen or non-frozen values.
3. Create a wrapper class that will store the modified value if a modifying operation is called, but otherwise recompute like normal. The issue with this is you need to know which operations cause mutations for every single type.

Number one is the direction we may be going for Chef resources; wherever we go, it's probably worth doing the same thing in both places so we can eventually share le code (for validation and defaults and all that jazz).

[lamont-granquist]

@mcquin you can look at Chef::Node::Attribute stuff and in particular the MUTATOR_METHODS and you basically wrap all of them to both proxy to the underlying object and to 'dirty' the setting so that it isn't considered a default any more. since you proxy the call to the underlying object you should be able to simply let NoMethodErrors bubble up from that call.

[jkeiser]

It is also worth pointing out that the user can call the setter for the value during default, causing it to be cached the first time it is set.