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 Fragments (Issue #36) #124
base: master
Are you sure you want to change the base?
Conversation
@kelvinqian00 This is excellent! I should have some time to review this in more detail in the next few days to get this merged in, but the first pass through it looks great. I like your careful attention to the fact that fragments differ from other parameters types since they must be considered/expanded before other parameter types. Thanks! |
@kelvinqian00 OK, I've had some time to look at this and I have few comments: I'm definitely seeing some test suite failures locally. To help see these failures in this PR, I've added a github actions workflow to run the test suite, and set up checks for PRs. You'll need to merge in the latest from
Secondly, I want to make sure that I understand the fragment registry. Is this registry global to the project? I think that could be an issue for larger projects. At first, it may seem like Clojure expressions have the same issue, but they are identified solely by their hash for the SQL string...which gives a singular compiled function for the SQL that will run. With fragments, which are identified by name, I think we must provide some consideration of the namespace. This would make the def'ing of the available set of fragments an explicit operation. If users want to have a set of
I'm not sure what's best, but I'm thinking either:
|
Regarding the failing test case, that's not because of an error in the fragments code, but an error with the test case itself. Specifically, one of the fragments was I was able to run the cases for all DBMSs with the exception for Apache Derby, which I couldn't get working for some reason. For the others, some of the instructions for Postgresql and MySql need to be updated; for example, the MySql user needs to be created before |
As for your namespaces question, yes, right now all fragments are stored in a global registry (you can see this as the single atom in The main issue I can think of, though, is how would namespaced fragment names be referenced in the .sql files. Would we want the names to always include the namespaces, or (similar to Clojure code) only need namespaces when referring to fragments from a different file/namespace? The former would be simpler to implement, but the latter might be what users would expect. |
GOAL: Add fragments, lines of SQL code that can be defined in
.sql
files in order to compose larger statements. This has been a long-requested feature; Issue #36 was created in 2016.USAGE: Fragments are designed to be similar to other parameters (particularly snippets) to a HugSql library user. A user defines a fragment similar to a HugSql function or snippet:
and can use it like a snippet:
Then when
frag-query
is defined or compiled, the fragments are automatically expanded into the full SQL statement:Fragments can also contain other fragments:
Fragments can also be written - and nested - in Clojure expressions:
Note that fragments in Clojure expressions are expanded during runtime, in contrast to other fragments.
DESIGN: Internally, fragments operate very differently from other parameters. This is not only because (non-Clojure expression) fragments are expanded during function definition, but because fragments are not given as parameters to HugSql functions. More details are given in the
fragments
namespace, including how the fragment registry works and how fragments are registered and expanded.TESTING: Tests for fragments have been added to the
test
namespaces. All core tests pass with H2 (and should pass on other DBMSs, though I have not tested that). Let me know if more tests are needed.EXTRA CHANGE: I also added a line to remove excess whitespace from the final SQL strings. The main motivation was that fragment expansion creates a lot of excess whitespace, but this should help clean up SQL that contains a lot of Clojure expressions as well.