grammarly / omniconf

Configuration library for Clojure that favors explicitness
Apache License 2.0
193 stars 16 forks source link

Issues with default value as false #5

Closed anujsrc closed 7 years ago

anujsrc commented 7 years ago
;; Define a single param conf1 of type boolean with a default value of false
user> (omniconf.core/define {:conf1 {:type :boolean :default false}})
user> (omniconf.core/verify)
Omniconf configuration:
 {}

nil

;; Now, set the default value as true
user> (omniconf.core/define {:conf1 {:type :boolean :default true}})
user> (omniconf.core/verify)
Omniconf configuration:
 {:conf1 true}

nil

;; Next, use a nested structure and see the behavior
user> (omniconf.core/define {:conf1 {:nested {:c1 {:type :number :default 0} :c2 {:type :string :default "c2"} :c3 {:type :boolean :default false}}}})
nil
user> (omniconf.core/verify)
Omniconf configuration:
 {:conf1 {:c1 0, :c2 "c2"}}

nil

;; Set default value to true
user> (omniconf.core/define {:conf1 {:nested {:c1 {:type :number :default 0} :c2 {:type :string :default "c2"} :c3 {:type :boolean :default true}}}})
nil
user> (omniconf.core/verify)
Omniconf configuration:
 {:conf1 {:c1 0, :c2 "c2", :c3 true}}

nil

Clarification

On the similar lines, why is the check on this line- https://github.com/grammarly/omniconf/blob/master/test/omniconf/t_core.clj#L81 not listing the default value of :one?

;; Current
(is (= {:first "alpha", :more {:two "two"}, :second 70} (cfg/get :nested-option)))

;; Why not?
(is (= {:first "alpha", :more {:one "one" :two "two"}, :second 70} (cfg/get :nested-option)))
anujsrc commented 7 years ago

If I define a top level default for nested ones, it does list but it is slightly confusing for me in case of the position of default values in both nested and other case. Here is what works for me for nested booleans-

user> (omniconf.core/define {:conf1 {:default {:c3 false} :nested {:c1 {:type :number :default 0} :c2 {:type :string :default "c2"} :c3 {:type :boolean :default false}}}})
nil
user> (omniconf.core/verify)
Omniconf configuration:
 {:conf1 {:c3 false, :c1 0, :c2 "c2"}}

nil
alexander-yakushev commented 7 years ago

Yeah, that's probably a bug. However, it might be argued that setting a boolean parameter ("a flag") is the same as not setting it at all. But there's no harm in fixing this.

anujsrc commented 7 years ago

Yes, agree. Here is the case where I find it relevant-

alexander-yakushev commented 7 years ago

Regarding:

(is (= {:first "alpha", :more {:two "two"}, :second 70} (cfg/get :nested-option)))

This is because --nested-option.more {} is in the test. It erases all defaults.

alexander-yakushev commented 7 years ago

I deployed this fix as 0.2.6. Hope this helps!

anujsrc commented 7 years ago

Thanks @alexander-yakushev. Pulled in 0.2.6. Works like a charm :+1: