Support default values via Block

[robacarp]

Hey folks,

I'm integrating this over at robacarp/mosquito, which is currently taking configuration from an environment variable. In my current situation, it would be nice to be able to emit a message when a default value is rendered. In this case it would be a deprecation notice:

module Mosquito
  Habitat.create do
    setting redis_url : String do
      Log.warn {
        <<-error_message
        Configuring the Mosquito redis connection via environment variable is deprecated as of 2020-11.

        The functionality will be removed in the next _minor_ version bump, 0.9.0.

        See: https://github.com/robacarp/mosquito#connecting-to-redis
        error_message
      }

      (ENV["REDIS_URL"]? || "redis://localhost:6379")
    end
  end
end

Maybe this would be otherwise useful, maybe not.

[jwoertink]

That's awesome you're integrating it. That should really help to configure Mosquito.

You can sort of do this now, but it's a little more code.

module Mosquito
  class HabitatSettings
    def self.default_redis_url
      Log.warn {
        <<-error_message
        Configuring the Mosquito redis connection via environment variable is deprecated as of 2020-11.

        The functionality will be removed in the next _minor_ version bump, 0.9.0.

        See: https://github.com/robacarp/mosquito#connecting-to-redis
        error_message
      }
  
      (ENV["REDIS_URL"]? || "redis://localhost:6379")
    end
  end

  Habitat.create do
    setting redis_url : String = default_redis_url
  end
end

Let me know if that would work for you. If so, then we can document this as an additional option.

[robacarp]

I like it. Cracking open the HabitatSettings class isn't exactly terse, but it's a good short term workaround. Thanks

[jwoertink]

Another thing I just thought of is you can actually just move that method to mosquito itself and not have to open the HabitatSettings class...

module Mosquito
    def self.default_redis_url
      Log.warn {
        <<-error_message
        Configuring the Mosquito redis connection via environment variable is deprecated as of 2020-11.

        The functionality will be removed in the next _minor_ version bump, 0.9.0.

        See: https://github.com/robacarp/mosquito#connecting-to-redis
        error_message
      }
  
      (ENV["REDIS_URL"]? || "redis://localhost:6379")
    end

  Habitat.create do
    setting redis_url : String = Mosquito.default_redis_url
  end
end

[robacarp]

Actually, I think both of these will evaluate regardless of the default value being needed which means the warning message will be emitted always as well.

[paulcsmith]

I think we should allow a default with a proc (I think it has to be a proc so you can specify a return type, but not sure). I think that will open up a lot of possibilities that are hard to do right now (like what you're proposing)

In this particular case though you could probably do what @jwoertink suggested with a tiny modification:

module Mosquito
    def self.default_redis_url
      Log.warn {
        <<-error_message
        Configuring the Mosquito redis connection via environment variable is deprecated as of 2020-11.

        The functionality will be removed in the next _minor_ version bump, 0.9.0.

        See: https://github.com/robacarp/mosquito#connecting-to-redis
        error_message
      } if ENV["REDIS_URL"]? # Added the if so it only show if the ENV var is set
  
      (ENV["REDIS_URL"]? || "redis://localhost:6379")
    end

  Habitat.create do
    setting redis_url : String = Mosquito.default_redis_url
  end
end

[robacarp]

@paulcsmith yeah, that's nearly identical to what I landed on yesterday.

I also ran across the difficulty in testing the integration with Habitat around a default value. This may be because I don't have a proper mock in place for testing configuration values. I couldn't conceive a way to test that the default value existed and was used when the configuration parameter was blank, nor could I conceive a way to test that missing parameters actually cause an appropriate raise.

I have a hunch that I could subclass and scaffold some configuration-meta for testing that different configuration parameter sets result in the appropriate way, but I couldn't get there in an afternoon.

[paulcsmith]

Nice! Glad you got it working

Thea ideal would probably be that Habitat has the feature built-in so you don't need to test it. Kind of like you probably wouldn't test that the getter macro works and sets a default like it should.

I think if we add in the ability to accept a Proc and if the return type is the expected type then you should be good to go without testing, but maybe I am misunderstanding the use case?

BTW you can have Habitat raise if a setting is missing if that's what you were trying to test:

habitat/src/habitat.cr

Line 54 in 38cc672

def self.raise_if_missing_settings!

[robacarp]

The reason for wanting the test is because that particular change is a full-stop-the-server-won't-work-anymore breaking change if the default value isn't in there -- the old behavior was implicit, like a lot of redis projects are, but the new behavior is implicit. My goal was to provide something which is backwards compatible for now, and raises a helpful warning, and is tested so that the backwards compatibility doesn't bitrot or otherwise evaporate without being noticed.

To put it another way, I trust Habitat is going to correctly apply my default value. I wanted a test that I correctly set the default value.

It's tricky because I already have the habitat config setup for the purpose of running the tests. The as yet undocumented testing helper is nice, but it doesn't provide a way to say "take the default value" like this imagine-code:

Mosquito.temp_config(redis_url: :default_value) do
    assert_equal <whatever goes here>, Mosquito.settings.redis_url
end

And perhaps that is a usable solution? It would work in my case, but I can't speak to it being useful enough beyond my own purposes.

[paulcsmith]

That makes sense! We should also document the temp_config because it is meant for exactly this purpose!

It is also super helpful for not just testing the settings but also overriding them in tests: https://github.com/luckyframework/lucky/search?q=temp_config

I think if that works in your case that should be good enough for now. But still agree on making a block a possible default!

Also if we allow a block/proc (probably a Proc) then the library can have a method for the proc like MyClass.default_redis_url which returns the proc. Then you can test the proc separate from Habitat to see if it raises or returns what you expect. Does that make sense?

[robacarp]

Yeah, I considered the MyClass.settings.default_config_var method as well, and returning a proc sounds good to me.

[jwoertink]

The block value should probably work similar to the getter / property macros where they are lazily evaluated. On top of that, the value should be memoized if it's not. I want to make sure that if your setting is creating some object (i.e. Redis connection), then it shouldn't be creating a new connection each time you call it. That maybe be tricky with the class level state, so maybe I open an issue separate on that.