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

cprop cannot handle setting nils #35

Open
bn-darindouglass-zz opened this issue Apr 17, 2020 · 2 comments · May be fixed by #36
Open

cprop cannot handle setting nils #35

bn-darindouglass-zz opened this issue Apr 17, 2020 · 2 comments · May be fixed by #36

Comments

@bn-darindouglass-zz
Copy link
Contributor

bn-darindouglass-zz commented Apr 17, 2020

So it turns out cprop cannot handle niling a setting. After some looking around and some testing I think the problem is just one of unnecessary complexity:

The str->value function tries to be smart and does some long/boolean/string regex matching before sending the value to edn/read-string. This processing is both unnecessary and can actually limit functionality as mentioned in the title:

cprop.source> (let [or-nil #(or % "nil")]
                (doseq [v ["hello" -1 100 100000000000000000000 -100000000000000000000 nil true false]
                        :let [cprop (str->value (str v) {})
                              edn (edn/read-string (str v))]]
                  (clojure.pprint/print-table [:parser :value :type]
                                              [{:parser "" :value (or-nil (str v)) :type (or-nil (type v))}
                                               {:parser "cprop" :value (or-nil cprop) :type (or-nil (type cprop))}
                                               {:parser "edn" :value (or-nil edn) :type (or-nil (type edn))}])))

| :parser | :value |                     :type |
|---------+--------+---------------------------|
|         |  hello |    class java.lang.String |
|   cprop |  hello |    class java.lang.String |
|     edn |  hello | class clojure.lang.Symbol |

| :parser | :value |                :type |
|---------+--------+----------------------|
|         |     -1 | class java.lang.Long |
|   cprop |     -1 | class java.lang.Long |
|     edn |     -1 | class java.lang.Long |

| :parser | :value |                :type |
|---------+--------+----------------------|
|         |    100 | class java.lang.Long |
|   cprop |    100 | class java.lang.Long |
|     edn |    100 | class java.lang.Long |

| :parser |                :value |                     :type |
|---------+-----------------------+---------------------------|
|         | 100000000000000000000 | class clojure.lang.BigInt |
|   cprop | 100000000000000000000 | class clojure.lang.BigInt |
|     edn | 100000000000000000000 | class clojure.lang.BigInt |

| :parser |                 :value |                     :type |
|---------+------------------------+---------------------------|
|         | -100000000000000000000 | class clojure.lang.BigInt |
|   cprop | -100000000000000000000 | class clojure.lang.BigInt |
|     edn | -100000000000000000000 | class clojure.lang.BigInt |

| :parser | :value |                  :type |
|---------+--------+------------------------|
|         |    nil |                    nil |
|   cprop |        | class java.lang.String |
|     edn |    nil |                    nil |

| :parser | :value |                   :type |
|---------+--------+-------------------------|
|         |   true | class java.lang.Boolean |
|   cprop |   true | class java.lang.Boolean |
|     edn |   true | class java.lang.Boolean |

| :parser | :value |                   :type |
|---------+--------+-------------------------|
|         |  false | class java.lang.Boolean |
|   cprop |    nil | class java.lang.Boolean |
|     edn |    nil | class java.lang.Boolean |
nil

I'm fairly certain simply removing the regex matching from str->value would only add functionality to the parsing. Anecdotally: before babashka supported cprop as a library, I mimic'd the behavior using just edn/read-string and it worked like a charm.

bn-darindouglass-zz pushed a commit to bn-darindouglass-zz/cprop that referenced this issue Apr 17, 2020
- The previous regex checks are redundant given `edn/read-string`
  provides the same functionality.
@tolitius
Copy link
Owner

the reason for str->value to front the Clojure reader is because not all configuration and properties come from edn like in your table above. Some of them, quite often, come from ENV, system properties or configuration managers (e.g. Consul).

In those cases types are not as clear. For example, let's look at overriding a "username":

=> (require '[cprop.core :as cp])
=> (cp/load-config)
{:datomic ...
 :source
 {:account
  {:rabbit
   {...
    :username "guest"}}}
=> (System/setProperty "source_account_rabbit_username" "0x42")

with the way code is written now:

=> (cp/load-config)
{:datomic ...
 :source
 {:account
  {:rabbit
   {...
    :username "0x42"}}}

relying on Clojure reader (i.e. removing "these lines"):

=> (cp/load-config)
{:datomic ...
 :source
 {:account
  {:rabbit
   {...
     :username 66}}}

I added better comments and a test so it clarifies things a bit

@bn-darindouglass-zz
Copy link
Contributor Author

Thanks for the quick turn-around!

Yeah there’s definitely some overlap where things can be parsed oddly; though I think the times where the reader would mis-parse something would be the vast minority situations.

Anyway, my original intent was lost among my thoughts on the use of the reader: you cannot nil a setting, it’ll be set to “nil” instead. A simple (= “nil” v) before the alpha number regex should fix the use case.

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 a pull request may close this issue.

2 participants