Not Nil fields refactor

[Blacksmoke16]

Having the field vs field! to determine if a column is not nil doesn't feel crystal like to me.

IMO, wouldn't it be more clear to base that off of the type of the field? For example:

class Test < Granite::Base
  adapter pg
  
  field name : String # Not nil
  field is_attending : Bool? # Nilable
end

This is essentially what the field! macro does, but then you could end up with using the bang version of field with a type of String. At a glance this makes it feel like it should be not nil, based on typing behavior elsewhere in Crystal.

[drujensen]

yes, I agree. not sure its possible though.

[c910335]

The idea of field! came from Object property!.

[drujensen]

Interesting. Didn't know Crystal had that.

Maybe we should keep this syntax and leverage the property! macro to define it.

field! name : String
field? age : Int32

[Blacksmoke16]

That's kinda how it works now, using field! sets a flag that changes the suffixes on the property/getter.

This is on my list to look into, ill post an update when i get some time to look into this more.

[Blacksmoke16]

@drujensen @c910335 I have a pretty good working prototype of this. However my current issue is how to handle the primary key. It's a catch 22 in that the PK can't be nil but it can be nil if its an unsaved model.

So far I have come up with some solutions:

1. Make the PK nilable.
   This would require the user to do like primary my_id : Int64?. It also would require the user to do nil checks when interacting with the PK. i.e. model.id + 1 # => undefined method '+' for Nil (compile-time type is (Int64 | Nil))
2. Make the PK nilable behind the scenes.
   This would require the user to declare their primary like primary my_id : Int64 (without the ?), but would still require nil checks for certain things.
3. Set the PK to a default value for a new record
   For example if its a Number PK type, then -1 or "" if its a String PK.
4. Some other option you think of.

[robacarp]

I vote for option 2. Nilable behind the facade.

[drujensen]

ditto. Seems like the best option at this point.