-
Notifications
You must be signed in to change notification settings - Fork 211
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
Fallback to use result of first branch when decoding :or and :orn #946
Conversation
src/malli/core.cljc
Outdated
(let [x* (transformer x)] | ||
(if ((nth validators i) x*) | ||
(reduced x*) | ||
(or acc x*)))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about when the first transformer
returns a nil
? Could that happen?
I'm thinking of something like (m/decode [:maybe :int] false false-to-nil-transformer) ==> nil
Or perhaps even just (m/decode [:or nil :int] nil trans)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it wouldn't matter, as [:or nil :int]
with nil
value succeeds and doesn't use the fallback but here's a case where is fails:
(m/decode
[:or
[:string {:decode/remove-secrets (constantly nil)}]
:keyword]
"SECRET!"
(mt/transformer {:name :remove-secrets}))
; => "SECRET!"
Fixed it so that value of the first branch is always used as fallback, regardless of value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change looks good, I like the tests. one question though.
from #910:
this PR implements option 2.
This is a BREAKING change as the behavior changes. But, I think the current implementation is wrong and this is (more) correct.