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.

Sign up for GitHub

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

Implement function criteria #912

Merged
merged 7 commits into from
Dec 2, 2022
Merged

Implement function criteria #912

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4ce8af4
Add updated? and created? methods on SaveOperations
davidepaolotua Oct 25, 2022
2c3da91
Merge branch 'main' of https://github.com/jinn999/avram into add-upda…
davidepaolotua Nov 30, 2022
5078688
Implement date function
davidepaolotua Dec 1, 2022
a1f93ad
Move upper and lower to new criterias
davidepaolotua Dec 1, 2022
09c82f5
generalize the application of functions to columns
davidepaolotua Dec 1, 2022
6774dae
Put in standby the extra-param function creation
davidepaolotua Dec 1, 2022
5985d25
Final clean up
davidepaolotua Dec 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions spec/avram/string_criteria_spec.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,12 @@ describe String::Lucky::Criteria do
end
end

describe "trim" do
it "uses TRIM" do
name.trim.eq("elon").to_sql.should eq ["SELECT #{QueryMe::COLUMN_SQL} FROM users WHERE TRIM(users.name) = $1", "elon"]
end
end

describe "not" do
describe "with chained criteria" do
it "negates the following criteria" do
Expand Down
5 changes: 5 additions & 0 deletions spec/avram/time_criteria_spec.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@ describe Time::Lucky::Criteria do
end
end

it "as_date" do
input_date = "2012-01-31"
activated_at.as_date.eq(input_date).to_sql.should eq ["SELECT users.id, users.created_at, users.updated_at, users.activated_at FROM users WHERE DATE(users.activated_at) = $1", input_date]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If someone passes a Time object to input date, will it do the right thing?

Copy link
Contributor Author

@davidepaolotua davidepaolotua Dec 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It actually depends on what is the right thing to do. At this moment, it will not be compiling, with this error

Error: expected argument #1 to 'String::Lucky.parse' to be Array(String), Nil or String, not Time

The problem is that the function silently switches the criteria to a string. It's mostly about making a choice on how to represent its outcome in Lucky. Date does not exists and unfortunately, that's what the function is returning. So I had to choose between

  • String
  • Time

Now the problem becomes that the eq() expects the other to be of the same type (thus the error), so I had to choose between .as_date.gte("2011-01-01") or as_date.gte(Time.utc), I opted for the first but I think it's really a matter of choices in there

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My initial instinct is that it would run the proper .to_s on the Time to get the date out as a string. I can conceive of ways that might not actually be the right thing to do -- though it is what I would have expected.

If there's enough ambiguity in the use case then sure, raising an error is probably the right thing to do. It's a good candidate for a more helpful error message rendered at compile time. If it's possible to lend advice on what actually is expected, that would be a good way to lead folks to the happy path. Eg "Did you mean .eq(time.psql_format)?" or whatever method it would actually be.

Copy link
Contributor Author

@davidepaolotua davidepaolotua Dec 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was an other alternative I was thinking about, but it risks becoming quite complicated after a while...
It basically consists of defining a new Date class, almost invisible to everyone but the Time::Lucky one, which would then redefine basically all operators with overloading (something like instead of having:
criteria(T).eq(other : T) we would have criteria(Date).eq(other : String) and criteria(Date).eq(other : Time).
But honestly I don't even know if this is feasible.

Still, this would open the doors also for the Interval class...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat idea, but it can wait. Your scope here is nicely consumable as is.

end

describe "extract" do
it "fails with non supported symbol" do
expect_raises(ArgumentError) { activated_at.extract(:dayz).eq(5).to_sql }
Expand Down
24 changes: 3 additions & 21 deletions src/avram/charms/string_extensions.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,6 @@ class String

class Criteria(T, V) < Avram::Criteria(T, V)
include Avram::IncludesCriteria(T, V)
@upper = false
@lower = false

def like(value : String) : T
add_clause(Avram::Where::Like.new(column, value))
Expand All @@ -40,25 +38,9 @@ class String
add_clause(Avram::Where::Ilike.new(column, value))
end

def upper
@upper = true
self
end

def lower
@lower = true
self
end

def column
if @upper
"UPPER(#{@column})"
elsif @lower
"LOWER(#{@column})"
else
@column
end
end
define_function_criteria(upper, V)
define_function_criteria(lower, V)
define_function_criteria(trim, String)
end
end
end
2 changes: 2 additions & 0 deletions src/avram/charms/time_extensions.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,8 @@ struct Time
class Criteria(T, V) < Avram::Criteria(T, V)
include Avram::BetweenCriteria(T, V)
include Avram::ExtractCriteria

define_function_criteria(as_date, String, "DATE")
end
end
end
7 changes: 7 additions & 0 deletions src/avram/criteria.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,4 +185,11 @@ class Avram::Criteria(T, V)
sql_clause
end
end

macro define_function_criteria(name, output_type = V, sql_name = nil)
{% sql_name = sql_name ? sql_name.id : name.id.upcase %}
def {{name}}
Criteria(T,{{output_type}}).new(rows, "{{sql_name}}(#{column})")
end
end
end
2 changes: 1 addition & 1 deletion src/avram/criteria_extensions/between_criteria.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ module Avram::BetweenCriteria(T, V)
def between(low_value : V, high_value : V)
add_clauses([
Avram::Where::GreaterThanOrEqualTo.new(@column, V.adapter.to_db!(low_value)),
Avram::Where::LessThanOrEqualTo.new(@column, V.adapter.to_db!(high_value))
Avram::Where::LessThanOrEqualTo.new(@column, V.adapter.to_db!(high_value)),
])
end
end
Expand Down
1 change: 0 additions & 1 deletion src/avram/define_attribute.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ module Avram::DefineAttribute
ATTRIBUTES = [] of Nil
\{% end %}


\{% if [email protected]? %}
\{% for attribute in @type.ancestors.first.constant :ATTRIBUTES %}
\{% ATTRIBUTES << attribute %}
Expand Down
2 changes: 1 addition & 1 deletion src/avram/model.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ abstract class Avram::Model
end

macro inherited
COLUMNS = [] of Nil # types are not checked in macros
COLUMNS = [] of Nil # types are not checked in macros
ASSOCIATIONS = [] of Nil # types are not checked in macros
include LuckyCache::Cachable
end
Expand Down