added query macro, tests, and docs #215
Conversation
| @@ -0,0 +1,21 @@ | |||
| module Granite::Select | |||
| struct Container | |||
There was a problem hiding this comment.
Could this be called a fieldset?
There was a problem hiding this comment.
It's not so much a set of fields as it is a (loose) container for the query (or SELECT) statement. Granite::Query is used in your new query builder, so I tried to capture this as simply as possible. I don't love Granite::Select::Container though. Open to ideas.
There was a problem hiding this comment.
Sure Container is a little vague, but is it going to show up in an Amber devs code? If not, I don't think it's important to be 100% on target for merging this.
There was a problem hiding this comment.
Makes sense. No, the only dev interface is the query macro, so not a big deal here.
src/granite/select.cr
Outdated
| module Granite::Select | ||
| struct Container | ||
| property custom | ||
| getter table_name, fields |
There was a problem hiding this comment.
Since you’re target is to support multiple tables, shouldn’t each field have a paired table?
There was a problem hiding this comment.
@robacarp there may be no tables or they may be fields that are calculated. When this is a custom query, the table_name will not have any meaning.
What I think may be a bigger issue is all of the transactional methods are still included when a custom query is provided. These methods should be excluded like save and destroy or at least throw runtime exceptions if you attempt to use them. But it would be better to catch these at compile time.
There was a problem hiding this comment.
@drujensen Agreed. I think the next step (and I'll write up an issue to discuss this) is to go with your original suggestion in #184 and fully break this out into a ViewModel, which will eliminate the transactional methods (and, hence, no need for compiler check).
src/adapter/sqlite.cr
Outdated
| stmt << " FROM #{quote(table_name)}" | ||
| stmt << " WHERE #{quote(field)}=:id LIMIT 1" | ||
| def select_one(query : Granite::Select::Container, field, id, &block) | ||
| if query.custom.size > 0 |
There was a problem hiding this comment.
change custom to a nil-able property and do not initialize it to "". property custom : String? Then check for nil here instead of size
There was a problem hiding this comment.
I think you can get rid of the if blocks with initial_statement = query.custom || String.build do |stmt|
There was a problem hiding this comment.
Great - thank you for suggestions. Done and done.
robacarp
dismissed
drujensen’s stale review
@drujensen I believe your concerns were addressed
This PR (#184 & #200) allows you to customize the SQL used in the SELECT portion of the query (or the entire query if necessary) by adding the
querymacro.The major limitation is that when using a highly-modified
query, the save/update functionality of the model will likely break. In the issue @drujensen suggested taking a more complete View Model approach, and I agree with that entirely, but wanted to start with a slightly narrower scope. So this is "View Model Lite". Baby steps. Likewise,findmay also break depending on how the query is modified.Also, naming is hard. I'm not wedded to any of the names used, and particularly with things like Query, Select, etc., things might get complex.
(PS - Please feel free to rip to shreds :) I'm new to this type of contribution so am eager to learn & improve in both style and substance. Happy to make as many changes as needed.)