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

Optional argument default-fn to mt/default-value-transformer #582

Merged
merged 3 commits into from
Feb 15, 2022

Conversation

piotr-yuxuan
Copy link
Contributor

See an example usage here: https://github.com/piotr-yuxuan/malli-cli/blob/main/src/piotr_yuxuan/malli_cli.clj#L220-L222

This doesn't change existing behaviour but only add default-fn optional argument. When set, this function is applied on the default value found under key key. In the example above this optional, additional indirection allows a seamless way to define :env-var defaults.

@piotr-yuxuan
Copy link
Contributor Author

I didn't expect the tests to fail when adding one optional argument atop upstream commit – but I guess it's what tests are for 😛 If there were any interest in this, happy to make tests pass, work out some real-life examples, and add it to the README.

See an example usage here:

https://github.com/piotr-yuxuan/malli-cli/blob/main/src/piotr_yuxuan/malli_cli.clj#L220-L222

This doesn't change existing behaviour but only add `default-fn`
optional argument. When set, this function is applied on the default
value found under key `key`. This optional, additional indirection
allows a seamless way to define `:env-var` defaults.
@piotr-yuxuan
Copy link
Contributor Author

Just amended the commit, which makes the update quite straightforward I guess.

@ikitommi
Copy link
Member

Looks good, could you add a test for this to avoid future regression.

piotr-yuxuan and others added 2 commits January 17, 2022 20:30
Test additional argument to default-value-transformer. It emphasizes
that the function depends on the transformer, not the schema.
@ikitommi ikitommi merged commit e298309 into metosin:master Feb 15, 2022
@ikitommi
Copy link
Member

changed this to 2-arity, passing in the schema too as first argument:

(m/decode
 [:map
  [:os [:string {:property "os.name"}]]
  [:timezone [:string {:property "user.timezone"}]]]
 {} 
 (mt/default-value-transformer 
  {:key :property
   :default-fn (fn [_ x] (System/getProperty x))}))
; => {:os "Mac OS X", :timezone "Europe/Helsinki"}

@ikitommi
Copy link
Member

#644

@piotr-yuxuan
Copy link
Contributor Author

Sorry for the broken tests, I should have been more engaged and proactively merge master into this branch. Thanks very much for the merge!

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.

2 participants