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

Print a warning when config cannot be found #35

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

noprompt
Copy link

@noprompt noprompt commented Mar 30, 2019

This modest patch presents a warning to the user whenever configuration cannot be found in the expected locations. The motivation for this stems from being frustrated by

Missing required value for config var: foo.bar/baz

and the subsequent giant pile of stack trace when the file is missing. In general, I think I would prefer a much better error message when config value is missing i.e. it couldn't be found because the configuration source was missing vs. it couldn't be found because it wasn't specified in the source. These are distinct situations and warrant distinct messages.

I think this could easily be improved by having load-config annotate the return value of (read-config source) with meta indicating it's source and subsequently providing a helper function to access that information. These additions would be enough to produce a better error message in defconfig.

(defn load-config
  "Finds and loads config."
  []
  (if-let [source (find-config-source)]
    (with-meta (read-config source) :source source)
    {}))

(defn config-source
  "Returns the source of config as string or nil if no source was
  found."
  []
  (when-let [source (:source (meta @config))]
    (str source)))

;; In defconfig.
(if (present? qname#)
  (alter-var-root var# (constantly (lookup qname#)))
  (when (and (-> var# meta :required) (not generating?))
    ;; This is the the change.
    (let [message# (if-let [source (config-source)]
                     (str "Config var " qname# " must define a default or be specified in " source)
                     (str "Config var " qname# " must define a default when there is no config source"))]
      (throw (Exception. message#)))))

I'm happy to revise and submit these proposals as a proper patch provided the reception is positive. The changes are safe, simple and would not result in breakage.

@pjstadig
Copy link
Contributor

pjstadig commented Apr 1, 2019

I think as long as we're covering all the bases it should be fine. A config can be required (or not) and have a default (or not). It is totally conceivable to have an application that specifies defaults for all its configs and it is normal that there is no config.edn except if you want to override some default.

A required config should cause the application to fail to start, if a value cannot be found for it. I think it is helpful in that circumstance to distinguish between "a config value could not be found" and "LOL there's not even a config file".

I guess that's all I have to say about it. :)

@timothypratley
Copy link
Contributor

Oh snap; source info is a great idea! :) +1

@noprompt
Copy link
Author

noprompt commented Apr 3, 2019

I’ll be adding tests for the changes soon. Been a bit busy!

@noprompt
Copy link
Author

Apologies for the long pause. I added a test for config-source which is the primary contribution here.

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

Successfully merging this pull request may close these issues.

3 participants