Add limit and offset to Builder/Assembler with some fixes #284
Conversation
There was a problem hiding this comment.
Thanks @c910335! I appreciate you refactoring and picking up the querybuilder where I left off.
I left one comment for discussion about the sql_builder.
The weird whitespacing is a combination of two features integrating: unified query logger and this query builder. Without the logger, the querybuilder as it was attempted to output formatted SQL, something like this:
SELECT field, field, field, field
FROM monitor_results
WHERE run_start_time = $1 AND host_id = $2
ORDER BY id DESC
Nevertheless, formatted SQL was more helpful for writing the query builder framework than it will be moving forward, so I'm fine removing it.
src/granite/query/assemblers/base.cr
Outdated
| def build | ||
| @clauses.join " " | ||
| end | ||
| end |
There was a problem hiding this comment.
This is a clever shortcut, but I'm not sure it needs to be a class. Why not just implement this in bulid_sql?
def build_sql(&block)
clauses = [] of String
yield clauses
clauses.compact.join ' '
end| sql = build_sql do |s| | ||
| s << "SELECT COUNT(*)" | ||
| s << "FROM #{table_name}" | ||
| s << where if where |
There was a problem hiding this comment.
because of the logic in build_sql, can these if suffixes be dropped?
| @group_by : String? | ||
|
|
||
| def where | ||
| return @where if @where |
There was a problem hiding this comment.
This code isn't fresh enough in my mind to remember why I had build_* methods instead, but assuming there isn't some DSL caveat I think this is a fine refactor.
|
@robacarp are you ok with merging this? I have time to do a patch release today and would like to include these if you approve. |
|
@drujensen sure, if you like. I think there's room for more refactor along the comments I made, but it's functional as is, I think |
I found that the query builder which was introduced in #132 is very powerful when I was building Shale.
But I think it needs some improvement.
src/granite/query/builder_methods.cr.Thanks @Thellior for finding this bug.
They are very useful for pagination.