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

add optional usage of hugsql options in bind-connection #27

Merged
merged 4 commits into from Oct 18, 2016
Merged

add optional usage of hugsql options in bind-connection #27

merged 4 commits into from Oct 18, 2016

Conversation

nottmey
Copy link
Contributor

@nottmey nottmey commented Oct 9, 2016

PR for #25

In bind-connection I needed to reuse part of the variadic parameter filenames to not break backwards compatibility. This feels a bit awkward because the possibility for options is hidden in the code, but we can go with it for simplicity.

Another solution would be to offer another differently named function for advanced "bind-connection" use. The signature may look like this:

(defn bind-connection-opts [conn options & filenames] ...)

Since this opens up more design space, I tried to anticipate other advanced behaviors:

  • It is not possible to explicitly define the namespace for generation (besides wrapping bind-connection in (binding [*ns* 'some.namespace] ...)
  • It is not possible to programmatically generate the filenames (e.g. (bind-connection *db* (file-seq (io/file (io/resource "seeding"))))), without wrapping bind-connection in clojure.core/apply.

So an advanced "bind-connection" function might also look like:

(defn bind-connection-with [conn namespace options filenames] ...)
; Usage:
(bind-connection-with *db* 'myapp.db {:quoting :mysql} (collect-sql-files))

Any preferences?

After we settled for a solution I can also add a test case for it.

@yogthos
Copy link
Member

yogthos commented Oct 9, 2016

I think I'm ok with current approach of passing an optional config map. The connection and the namespace could be passed as part of the options map. As long as that's documented, I don't think it should be a problem. However, using a separate bind-connection-with macro might be a good idea to keep it explicit. I'd be ok with either approach.

@yogthos yogthos merged commit 6b75639 into luminus-framework:master Oct 18, 2016
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.

None yet

2 participants