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

Fix require-sql to use . instead of / in internal ns name #149

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

joelittlejohn
Copy link

It appears that the require-sql macro doesn't currently work, as it places the functions it defines into an invalid namespace.

Using the macro like:

(require-sql ["queries.sql" :as foo])
(foo/my-query ...)

Results in:

java.lang.IllegalStateException: Attempting to call unbound fn: #'yesquire/queries.sql/my-query

since the internal namespace created to hold the functions has an invalid char / within it (yesquire/queries.sql).

The tests in yesql.core-test also exhibit this problem, but since the functions aren't invoked the problem isn't obvious.

This change fixes the internal namespace in which the query functions are placed by using . rather than / chars in the ns name. I've also added an actual invocation of the declared query fns into the test to verify that the functions can be called.

@joelittlejohn
Copy link
Author

@iantruslove would you be able to take a quick glance at these changes and give this a thumbs-up if you think it's good to merge? Please shout if you think there's something I'm missing about this.

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.

1 participant