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

better match the behavior of assoc-in #33

Open
bn-darindouglass-zz opened this issue Jan 9, 2020 · 2 comments
Open

better match the behavior of assoc-in #33

bn-darindouglass-zz opened this issue Jan 9, 2020 · 2 comments

Comments

@bn-darindouglass-zz
Copy link
Contributor

Currently cprop assumes that all parts of a key path are keywords. Because of this assumption, only nested maps may be overridden by ENV vars, System properties, et al.

However, cprop uses assoc-in to make these updates which can support indices if the current root level is a non-map:

user=> (def servers {:redis [{:url "hi"}]
  #_=>               :es [{:url "world"}
  #_=>                    {:url "!"}]})
#'user/servers
user=> (assoc-in servers [:redis 0 :url] "hello")
{:redis [{:url "hello"}], :es [{:url "world"} {:url "!"}]}

I propose one of two options:

  1. If a part of a path matches #"^\d+$" then it should be treated as a long instead of a keyword to better align with assoc-in's behavior.
  2. Provide a way to inject a parsing function into the k->path function so users can parse paths in ways they would expect.
@bn-darindouglass-zz bn-darindouglass-zz changed the title allow for assoc-in vectors better match the behavior of assoc-in Jan 9, 2020
@tolitius
Copy link
Owner

the problem with the first regex solution is that the key name might become ambiguous: i.e. "0" vs. 0 vs. :0, etc.

you can substitute redis url in your example above using a nested map:

=> (c/load-config)
{:redis [{:host "localhost"}
         {:host "openstack"}]}
=> (c/load-config :override-with {:redis {0 {:host "prod"}}})
{:redis [{:host "prod"}
         {:host "openstack"}]}

let me know if it works for you

@bn-darindouglass-zz
Copy link
Contributor Author

Yeah the ambiguity is definitely a problem with the first suggestion; however I'd almost argue using any ^\d+$ key in a nested map is poor convention.

As for your example, my suggestion primarily comes from values set at the environment level. We have nested config that (unfortunately) has some required order about it, hence the vector inside the config structure. As it stands now the only way to override any value inside that vector is to override the whole key containing the vector, which can get unwieldy.

We could do as you suggested but that would mean having to have a sort of "meta" config that'll read from the environment and get merged into the "real" config at load time. I'd imaging that'd get pretty confusing.

bn-darindouglass-zz pushed a commit to bn-darindouglass-zz/cprop that referenced this issue Feb 21, 2020
This allows for custom parsing of each part of a key's path. One
example of a use-case is to better match the behavior of
`assoc-in` (i.e \d+ being treated as indices rather than keywords)

- Add new parse-fn to source.cljc
- Add new tests around this option
- Use `testing` instead of comments before `is` forms
bn-darindouglass-zz pushed a commit to bn-darindouglass-zz/cprop that referenced this issue Feb 21, 2020
bn-darindouglass-zz pushed a commit to bn-darindouglass-zz/cprop that referenced this issue Feb 21, 2020
- Update key-part parser option name
- Move number parsing function into tools namespace
- Update README with direct example
bn-darindouglass-zz pushed a commit to bn-darindouglass-zz/cprop that referenced this issue Feb 21, 2020
- It was private before so it shouldn't be a breaking change.
tolitius added a commit that referenced this issue Feb 22, 2020
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

2 participants