Fields are now required in default. Use foo? for nilable fields.

[maiha]

In this PR, field requires values ​​by default, otherwise raises the proper missing exceptions.
Now, users can avoid the redundant not_nil!, and also can use foo? for the nilable cases.

user = User.new(name: "maiha")
user.name? # => "maiha"
user.name  # => "maiha"

user = User.new
user.name? # => nil
user.name  # => User#name cannot be nil (Exception)

Best regards,

[drujensen]

@maiha nice solution. Thanks for submitting it.

I was wondering, should we allow for nilable declaration in the mapping itself?

field count : Int32?

wdyt?

[maiha]

Yep. Although there are various preferences, I think that it is better for the API to be consistent in each layers. That is,

• all fields must be declared as field foo : T (not T?)
• this means foo is saved as T in persistent layer if exists
• but we can use it as T? in programming layer for convinience

Otherwise, we must check its declaration every time we wonder whether it should be foo or foo? for each fields. I think it is very troublesome. 😿

[elorest]

This appears to be a pretty serious breaking change. Not anytime a user were to try to print or use pet.name it will error if the pet doesn't have a name unless they add pet.name?. This will be far less pleasant for the end user.

[faustinoaq]

@elorest What about just documenting it?, I mean, we already have been using this change on amber v0.6.5 and v0.6.7, and no amber developer was complaining about this 😅

[elorest]

We need to discuss this with @drujensen. My team is still using 0.6.4 and I don't think anyone has really tested the latest versions.

[drujensen]

@elorest I agree that this is a bigger breaking change than I anticipated. I didn't grasp the impact when I merged it.

I tried using 0.9.0 with amber cli and it will require changing all the fields in the forms and find methods to use?. This is not compatible with Crectco so we will need to adjust the templates to support both.

I do like the convenience of not requiring .not_nil! but I think the cost may be too high. WDYT?

[elorest]

I very rarely have to use .not_nil! and in most cases I prefer for it to possibly be nil. So I can do cool things like cat.name || "your cat doesn't have a name" OR just straight up == cat.name and have that print something or nothing in my template without having it exception because someone didn't set a name.

[drujensen]

@maiha should we roll this change back?

[robacarp]

I think it’s worth pulling back this feature. The right way may be to rollback this commit or not but this level of breakage is not something that should be done without major buy in across the amber ecosystem.

I’m with @elorest on this, it’s uncommon that I’ve ever had to use .not_nil! To resolve this. An alternate crystal prescedent exists to strip the nil away and is more alike the rails style which is to add an exclamation to the getter which will throw: field!

[faustinoaq]

Would be nice to read @maiha comments/opinions/suggestion about this 😅

[maiha]

I believe that this specification makes most users happy as I commented in https://github.com/amberframework/granite-orm/pull/146#issuecomment-377668234

current behavior

# for users who desires the field should not be nil
cat.name  # simple!

# for users who desires the field would be nil
cat.name? || "your cat doesn't have a name"

Otherwise, the former users will be forced ugly and unnecessary not_nil! hell as it was in previous versions.

previous behavior

# for users who desires the field should not be nil
cat.name.not_nil!  # why I must check nilable in spite of DB has not null constraint.
id = User.create.id.not_nil!  # why I must check primary key exists

# for users who desires the field would be nil
cat.name || "your cat doesn't have a name"

This old behavior make the former users always unhappy.

• https://github.com/amberframework/granite-orm/issues/57
• https://github.com/amberframework/granite-orm/issues/91

But, of course, if the community determines that the previous behavior is desirable, we should revert these commits or rollback to 0.8.4. In that case, I use my fork so there is no problem at all.

[faustinoaq]

@maiha Thank you for your comment!

What about this:

# for users who desires the field should not be nil
cat.name! # notice the admiration symbol

# for users who desires the field would be nil
cat.name || "your cat doesn't have a name"

[drujensen]

@faustinoaq I like this proposal. This avoids the breaking change. I think we can accomplish this by renaming the methods.

@maiha does this work for you? We would hate to see you use your branch and we definitely hate unhappy developers. Right now, I'm unhappy because this change breaks Amber in more ways than I originally envisioned. We can't integrate v0.9.0 in the templates because other ORM's do not support this approach.

[maiha]

@drujensen I am sorry for interfering with cooperation between v0.9 and Amber. Is it difficult for Amber to prepare different generator templates for each ORMs? If it's not easy, let's revert this commit ASAP!

[drujensen]

@maiha No, we are not going backwards. Check this out! https://github.com/amberframework/granite-orm/pull/171