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

merge-configs doesn't allow explicit set of nil value #29

Open
werenall opened this issue Oct 23, 2019 · 6 comments
Open

merge-configs doesn't allow explicit set of nil value #29

werenall opened this issue Oct 23, 2019 · 6 comments

Comments

@werenall
Copy link
Contributor

merge-configs function, deeper down, uses a pick-prioritized function that, among other scenarios, picks a non-nil value over a nil 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 to nil (like for :logger for instance). And sadly this is not possible:

(duct.core/merge-configs
  {:duct.database.sql/hikaricp {:logger nil
                                :something-else :yay}}
  {:duct.database.sql/hikaricp
   ^:demote {:jdbc-url "123"
             :logger "999"}})
=> #:duct.database.sql{:hikaricp {:jdbc-url "123", :logger "999", :something-else :yay}}

Probably at one point Duct should be using a contains? function instead of nil? or some? 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 🤔

@werenall
Copy link
Contributor Author

Now I see that it's somewhat a duplicate of this issue on weavejester/meta-merge

@werenall
Copy link
Contributor Author

werenall commented Oct 23, 2019

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?

@weavejester
Copy link
Contributor

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.

@werenall
Copy link
Contributor Author

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?

@weavejester
Copy link
Contributor

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.

@werenall
Copy link
Contributor Author

(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 []}}

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