Closed drew-patel closed 3 years ago
@drew-patel thanks for submitting this PR!
A couple of suggestions:
defhelper
only to update the query map with the constraint clauses and not to construct the SQL function call. We want to utilise the format-clause
multimethod to construct the function call so that it works with honeysql's priority-based ordering rule system. Also, add a priority to the constraint clause, which I think would be a number slightly larger than the priority assigned to with-columns
. I just noticed that with-columns
doesn't have a priority assigned to it either, but it will need one now. Could you please add one for it as well? constraint
so that going forward we're able to use this function for defining multiple constraints. Eg:
(constraints [:unique [:product_no :product_sku]]
[:primary-key [:name]])
:constraints
to the query map instead of assoc
ing the constraints to the collection of columns. It otherwise ends up treating each constraint as a column.@scimetfoo Thanks for the feedback and suggestions!
All the points are clear. I'll ask questions if I'm unsure about anything while working on them. Will get back to you in a few days with the changes.
@scimetfoo I have addressed comments 1-3 as described above. I've only implemented the solution for UNIQUE
and PRIMARY KEY
so far since they behave similarly.
I have no idea why I'm getting an extra space after the table name. See one of the tests.
Also, I'm not sure if the priority numbers chosen are ok. Happy to fix those if they aren't.
Thanks!
@scimetfoo Please let me know if the examples are sufficient. Thanks again for the review!
Related issue - #15
defhelper
which adds a top-level:constraints
key to the complete-sql-map:with-columns
into its own function so that it can be reused in theconstraints
format-clause method as well:with-columns
to construct the SQL string:constraints
to construct the columns and constraints string wrapped in the same set of parens