Fix comparisons to nil in queries & make such queries explicit
#46
Conversation
Will now throw an error at compile-time if one writes something like: ``` UserQuery::BaseQuery.nickname.is(nil) ``` The methods `is?`/`is_not?` exist for comparing to possibly `nil` values.
`IS NULL`/`IS NOT NULL` is kind of a special case, so this makes a bit more sense.
|
@paulcsmith Continuing on some questions from the previous PR:
I think you're totally right here. Let's get rid of them. Do we want to eliminate |
hibachrach
force-pushed
the
hib-make-nil-queries-explicit
branch
from
6d9c03f
to
e028dce
Compare
The question mark already indicates "returns Bool" or "can return Nil", so it makes sense not to overload it with a third additional meaning This also eliminates `is_not`/`is_not?` as they are redundant--one can simply use `.not.eq`/`.not.nillable_eq` instead.
hibachrach
force-pushed
the
hib-make-nil-queries-explicit
branch
from
e028dce
to
46858af
Compare
|
Hmm great question. I kind of like the shorthand “not” method. But at the same time I like the explicitness of “not.eq” I think we should try removing it and we can add it back later if we get sick of typing “not.eq”. Does that sound good? |
|
Sounds grrrrreaaat! |
This was simply a shortcut for `Avram::Criteria#not.eq(T)`, which is not significantly longer and is a bit clearer.
There was a problem hiding this comment.
Just one tiny comment. This looks awesome! Could you also add something to the change log for Lucky? https://github.com/luckyframework/lucky/blob/master/CHANGELOG.md#in-master-but-no-released-since-v012
Something like:
isin queries has been renamed toeq. For example:UserQuery.new.name.not.is("Emily")should now beUserQuery.new.name.not.eq("Emily")
src/avram/criteria.cr
Outdated
| def not(value) : T | ||
| is_not(value) | ||
| def nilable_eq(value) | ||
| add_clause(Avram::Where::Equal.new(column, V::Lucky.to_db!(value))) |
There was a problem hiding this comment.
I think this might make more sense with the method body swapped with eq. So this method body would replace eq and this method (nilable_eq(value)) would call eq(value). Does that make sense?
It makes sense to have `eq` have the primary business logic here
|
@paulcsmith Should I open up a separate pull request for that? |
|
@HarrisonB Yes that'd be great! It would be on the Lucky repo and not Avram. I'm trying to keep all Lucky framework relevant changes there so people using master now what changed and so when I make upgrade notes I don't forget anything :P |
|
Thanks for tackling this 🎉 :D |
Continuation/migration of luckyframework/lucky_record#289. See there for details.
Closes #38.