Implement function criteria #912
Conversation
Implements luckyframework#888. Adds two new boolean methods to SaveOperation. They work similarly to new_record? but always return false if the record was not persisted (e.g: failed save or not-yet-performed operation)
…ted-and-created-to-save-operations
| @@ -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] | |||
There was a problem hiding this comment.
If someone passes a Time object to input date, will it do the right thing?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
Neat idea, but it can wait. Your scope here is nicely consumable as is.
There was a problem hiding this comment.
This is amazing! I only had a brief min to look over it, and I like what I'm seeing. I assume there's already specs using upper and lower, so the interface didn't change? I don't have a lot of time to really dive in, but I'll try and come back and play with it while others have time to review.
Thanks for taking this on 🙌
The idea of this PR comes from #909.
This PR implements some two new functions, and removes the thing I hated about the string_extensions (@upper and @lower).
In sinthesis, the new stuff is:
define_function_criteria)as_datemethod (the one of Query Time columns by Date #909) for Timestrimmethod for Stringsupperandlowermethods in strings.I was wondering whether to have at look at multi-param functions, but currently there are some problems with placeholders :P (aka
coalesce(the_age,4)hardcodes the 4 in the query which is not good, so it's currently on standby