Open werenall opened 4 years ago
Now I see that it's somewhat a duplicate of this issue on weavejester/meta-merge
I just realized that that nil
can be forced by using a replace
meta key:
(duct.core/merge-configs
{:duct.database.sql/hikaricp {:logger (duct.core.merge/replace nil)
:something-else :yay}}
{:duct.database.sql/hikaricp
^:demote {:jdbc-url "123"
:logger "999"}})
=> #:duct.database.sql{:hikaricp {:jdbc-url "123", :logger nil, :something-else :yay}}
But I think that if this is expected then maybe the tests should include it to avoid confusion in expectations?
Sure; I don't mind another test. As for why this is the behaviour in the first place, meta-merge is mimicing the behaviour of Leiningen's profile merging.
Hmm... An update to what I said about the workaround using #duct/replace
:
I don't have time to debug now but sometimes the hikaricp
logger
overwrite doesn't work despite the replace. Could it be because I the right hikaricp config comes not from explicit call but from duct.module/sql
?
Can you give me a sample code that demonstrates the behaviour you're seeing? Duct modules should prefer your configuration over the defaults they add.
(duct/prep-config
{:duct.profile/base
{:duct.database.sql/hikaricp {:logger (duct.core.merge/replace nil)}}
:duct.module/sql {:database-url "123"}}
profiles)
=>
{:duct.database.sql/hikaricp {:jdbc-url "123", :logger #integrant.core.Ref{:key :duct/logger}},
:duct.migrator/ragtime {:database #integrant.core.Ref{:key :duct.database/sql},
:strategy :raise-error,
:logger #integrant.core.Ref{:key :duct/logger},
:migrations []}}
merge-configs
function, deeper down, uses apick-prioritized
function that, among other scenarios, picks anon-nil
value over anil
value. I suppose this is to deal with k-v pairs missing in one of the maps. But there are scenarios when one might want to explicitly set value of something tonil
(like for:logger
for instance). And sadly this is not possible:Probably at one point Duct should be using a
contains?
function instead ofnil?
orsome?
at some point. I'm looking at these two lines.Would you consider that a bug? merge-configs fn tests don't include it so I'm not sure 🤔