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

change defqueries to return table of fns #2

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jonesmelton
Copy link

This changes suresql/defqueries to return a table of queries, as well as defining them globally. I considered using a :local flag in the options struct to switch between the two, but since the return value is currently always nil I don't believe this breaks any of the API. I can still switch it back to that if it's preferable though.

Uses:

  • Queries can be passed around in normal bindings as function arguments, in app middleware, etc instead of in dynamics. This is my use case, since circlet wipes dynamics even if with-dyns is used.
  • Can bind queries "as needed" rather than up front, which can simply code using threads. The db connection can't be marshaled across a thread boundary (and would probably be a bad idea anyway see 2.6). Having the functions available in lexical scope gives more options for how to mitigate that.
    • Another approach could be to separate query parsing and binding them to a connection. They could take the connection at invocation time. This felt like a significant change to the approach. I think the API could be kept the same by using partial application carefully, but it would be more complex than what I did here.

I don't know why I had to change the imports in the tests, I was getting "could not find module" errors. I'm new to janet and have only used import with a relative path symbol, not a string. Maybe it's just an incompatibility with an older version? I'm on 1.25.1-meson. Again if that style is preferred I'm happy to change it back.

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