Skip to content
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

discussion: raise_if_missing_settings #55

Open
robacarp opened this issue Oct 29, 2020 · 3 comments
Open

discussion: raise_if_missing_settings #55

robacarp opened this issue Oct 29, 2020 · 3 comments

Comments

@robacarp
Copy link

robacarp commented Oct 29, 2020

I really appreciate the engagement and open discussion. On #54 the discussion started to wander into two territories. I'm opening this second issue to share my recent experience with raise_if_missing_settings as an outside implementer.

I ran into two roadblocks with validating config viability: 1) misunderstanding of the method receiver (Habitat vs MyClass); 2) lack of ability to set my own helpful error messages.

method receiver (Habitat vs MyClass)

I spun for a bit after reading this:

# At the very end of your program use this
# It will raise if you forgot to set any settings
Habitat.raise_if_missing_settings!

"At the very end of your program"? I think I want it before I start doing anything but after initialization.

I then wrote MyClass.raise_if_missing_settings! and spun for a while about why that was giving me compile errors. It makes sense that Habitat isn't polluting the methodspace of my class, but I was still caught off guard.

my own error messages

I'm a big fan of Lucky error messages. I reside in the Java world daily and often run across this helpful error: NullPointerException accompanied by a 2400 line stack trace. Great, thanks java. 🤦‍♂️ So I strive to be better and Lucky is a great reminder of how to do that.

This is how I ended up validating my critical setting:

Some code to invoke the validation, along with one other place and a memo-method to validate the setting

  @@settings_validated = false

  def self.validate_settings
    return if @@settings_validated
    @@settings_validated = true

    if settings.redis_url?.nil?
      message = <<-error
      Mosquito cannot start because the redis connection string hasn't been provided.

      For example, in your job runner:

      Mosquito.configure do |settings|
        settings.redis_url = "redis://localhost:6379"
      end

      See Also: https://github.com/robacarp/mosquito#connecting-to-redis
      error

      raise message
    end

    # just in case
    Habitat.raise_if_missing_settings!
  end

Maybe I've done it wrong, or maybe I'm just in overkill territory. There isn't much I can easily see Habitat could improve on this -- the benefit of any sort of dsl to manage the messages themselves is probably marginal. If some other project is going to follow this pattern, they're going to end up duplicating only about 5 lines out of 20 (nos: 50, 53,54,56, and 73). The other 75% is just my own prescriptive ramblings.

Anyway, thanks again for the library. 🍌

@paulcsmith
Copy link
Member

paulcsmith commented Oct 30, 2020

Thanks for opening this! For problem 1 I think we can just document it clearly

For problem 2

We do have an example and validation option. example is for when the setting is missing, and validation is for when it is present and you want to validate that it looks correct.

In your case I would use example and pass it that heredoc string. That will then show up when the setting is nil and Habitat.raise_if_missing_settings is called.

I can't remember if we document example and validation but we should :P

@jwoertink
Copy link
Member

Just to add to the validation part, we have Habitat.raise_validation_error("your custom error message here").

@robacarp
Copy link
Author

libraries should not call this method

Good to know. In the case of mosquito, it's both a library and borderline a stand alone application. In the library, it needs to connect to redis to create tasks to be consumed, and in the worker to consume them. I struggled with implementing the worker a bit, and I don't know how to make it easier. I wanted to just provide an entrypoint in the shard itself so no code is required but the worker needs access to the source code of the application -- and now a configure block is also required:

require "./my_application/*.cr"

Mosquito.configure do |settings|
  settings.redis_url = "redis://path-to-your-redis:6379"
end

Mosquito::Runner.start

But recommending devs put their own Habitat.raise_if_missing_settings in there above Mosquito::Runner.start feels unnatural to me -- maybe it shouldn't. Maybe I should just implement some validations? It's easy to move the doc into the example, and that feels right.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants