Fix comparisons to nil in queries & make such queries explicit

[hibachrach]

Fixes #123

Not totally happy with is?/is_not? as method names as it overloads a third additional meaning for ?-suffixed methods (the first two being (1) methods which can return nil and (2) methods which always return booleans) so I'm open to ideas there.

[hibachrach]

How would people feel about changing

is -> eq
is_not -> not_eq
is? -> is
is_not? -> is_not

This way when you say eq, it's clearer that you're always getting an = in SQL where is will intelligently resolve to an = or an IS in SQL?

[mikeeus]

Thanks a lot for working on this! All the specs are set up and its looking good except for the comment I made about the LuckyRecord::Where::ComparativeSqlClause class. I would love to hear your thoughts on it :)

[mikeeus]

I think I understand why you introduced this class. Inheriting from SqlClause and overriding the constructor to only accept the @column will throw an error unless you make @value nilable, which would break the class.

Your solution does work well to separate the two concepts, but I think clauses that reference NULL are a special case and should inherit from SqlClause instead of the other way around.

You can achieve this without changing SqlClause by introducing a NullSqlClause class and removing @value from the constructor by setting it to `"NULL":

abstract class SqlClause
  getter :column, :value

  def initialize(@column : Symbol | String, @value : String | Array(String) | Array(Int32))
  end

  abstract def operator : String
  abstract def negated : SqlClause

  def prepare(prepared_statement_placeholder : String)
    "#{column} #{operator} #{prepared_statement_placeholder}"
  end
end

abstract class NullSqlClause < SqlClause
  @value = "NULL"

  def initialize(@column : Symbol | String)
  end
end

The Null and NotNull classes can then use the default prepare method to generate their sql so you can set their operators to "IS" and "IS NOT":

class Null < NullSqlClause
  def operator
    "IS"
  end
  ...
end

I believe this also means you can use the normal prepared_statement_values and joined_wheres_queries methods.

[hibachrach]

Hmm yeah I like this idea better. It's likely to be the only clause of its sort.

[hibachrach]

One thing: I'm not sure how you can get NullSqlClause to use the normal prepared_statement_values as that would result with users.nickname IS NULL $1.

EDIT: This seems like a classic case of the Circle-Ellipse problem 😛

[mikeeus]

If you change the operator to "IS" it should come out like users.nickname IS $1 and work as expected, right?

[hibachrach]

Ah yes. Sorry!

[hibachrach]

Sorry to keep asking questions about this, but won't passing in NULL as a dollar-sign param. result in it getting escaped? And it just being treated like the string "NULL"? And we can't override the ivar in the child class to set it to a nil literal.

[mikeeus]

It's no problem, I missed the details and oversimplified it. I think you can do with another prepare method then :)

def prepare
   "#{column} #{operator} #{@value}"
end

[mikeeus]

Regarding the naming of is? and is_not? I agree that it might be getting a little confusing. I like the usage of ? to mean "can return a nil value". I like the idea of renaming it to eq and so on. I'd like to hear @paulcsmith ideas on this

[paulcsmith]

@HarrisonB Thank you for working on this! I still need to look at the code, but I am happy you proposed some new syntaxes. As you discussed here and in https://gitter.im/luckyframework/Lobby?at=5c32c11c6370df0b919feb27 I think ? is a bit ambiguous. I do like changing is -> eq a lot.

I think we should go with:

is -> eq
is_not -> not_eq
is? -> nilable_eq
is_not? -> not_nilable_eq

And actually I'm wondering if we even need is_not since we have .not.eq and .not.nilable_eq.

Thoughts on removing the *_not version? Any other thoughts on this?

[edwardloveall]

Hey @HarrisonB! We've moved and renamed LuckyRecord to Avram (thanks for the name idea!). We decided to move it instead of using GitHub's rename option so that current code that relies on lucky_record would continue to work and a project must opt-in to the new shard. However, a big bummer is we can't move PRs.

If you are still interested in this change, would you be able to rebase onto Avram? If not don't worry about it. We can eventually rebase it for you. Thanks 😄! Let us know if you have any questions.

[hibachrach]

Certainly!

I’m a bit surprised GitHub doesn’t just have a simple rename functionality for repositories.

Hopefully it’s just a one-time thing haha

[paulcsmith]

Yeah there is a rename function but I’ve heard that it sometimes causes weird issues with got remotes and messed up links :S And yes this won’t happen again!

…

On Feb 2, 2019, at 6:12 PM, Harrison Bachrach ***@***.***> wrote: Certainly! I’m a bit surprised GitHub doesn’t just have a simple rename functionality for repositories. Hopefully it’s just a one-time thing haha — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.