find,find_by,first now ensures T, call with trailing ? for T?

[maiha]

This PR make class query methods to ensure a value exist in default like #144.

• find, find_by, first return T if exists, otherwise raises NotFound
• find?, find_by?, first? return T?

User.find?(1) # => #<User:0x99a300 @id=1 ...
User.find(1)  # => #<User:0x99a300 @id=1 ...
User.find?(0) # => nil
User.find(0)  # raises Granite::ORM::Querying::NotFound.new("Couldn't find User with id=0")

This removes many not_nil! codes as in this commit.
Also this prevents newbies from asking

• why undefined method 'xxx' for Nil?

[robacarp]

@amberframework/granite-orm-contributors it seems like there are a couple different paradigms in Crystal about this. Some people want to use:

• method? to return a T?
• method to raise on T.nil?

and others want:

• method to return a T?
• method! to raise on a T.nil?

I personally prefer the latter, but only because of my history with Ruby/Rails. I expect ? methods to return a boolean, and ! methods to do something dramatic: raise, destroy data, etc.

Do we have a paradigm for this in Amber projects? Any other preferences?

[drujensen]

oops. wrong button.

[drujensen]

@robacarp @maiha crystal follows the swift optional paradigm. Swift calls their union type a optional wrapper but the concept is the same. The variable can be either T or Nil. So if you declare something with a type T? in crystal and swift, you have a union type of (T || Nil)

In crystal, not_nil! is the same as ! in swift. It unwraps or removes the Nil from the union type. I requested that they change this to match swift's syntax, but the crystal team wants it to be ugly on purpose because they believe its a bad practice to not check for nil.

I think this request is trying to perform optional chaining where it will gracefully fail if the value is nil. This is a technique used is several languages. The ruby use of ? and ! doesn't fit well here.

I am ok with this change since it goes hand in hand with the last change we made but I think this should trigger a bigger discussion with the core team if we want to try and handle this stuff or if its better suited in Crystal itself.

[drujensen]

Here is a good article that explains Swift's Optionals. Scroll down to the optional chaining to see how swift handles it.

https://hackernoon.com/swift-optionals-explained-simply-e109a4297298

[robacarp]

I think this should trigger a bigger discussion with the core team if we want to try and handle this stuff or if its better suited in Crystal itself.

@drujensen Thanks for the background and the link. I think regardless of what Crystal Lang decides to do in the near or far term, Amber needs to be consistent with this stuff in the API. We are at the stage where we will benefit from setting standards about this sort of thing. For example, does this pattern jive with what’s being written for the params refactor? I honestly haven’t looked yet. But I think the projects should be consistent in its opinions about this type of syntax.

@maiha thank you again for your submission

[robacarp]

For the release notes, is this a breaking change?

[drujensen]

@robacarp no, I think the find, find_by, etc. works as it did before.

[drujensen]

@robacarp @maiha I was wrong, it looks like this is a pretty big breaking change. I was under the impression find and find_by would work as they did before and we were adding a find? and find_by?. This is not the case and something we should consider rolling back. WDYT?

[maiha]

First, I know that this and #144 is a very big API change. But, I think that the specification where T is default is convenient from the following three points.

1. Since it is natural that we expect that data exists in the persistence layer
2. Since not_nil! is long and ugly, the conversion cost of T? → T is high in Crystal
3. It seems very natural API to use T and T? properly according to the expectation of existence of data

This is a very big change, but I believe it is also a very big improvement.

[faustinoaq]

@maiha Good points 👍 , what about using T! to raise an exception and making T niliable by default 😅

[drujensen]

Please review https://github.com/amberframework/granite-orm/pull/171. This fixes the backward compatibility and changes to use the ! for .not_nil! option.