entc/sql/decode: fix decoding of NullTime in optional and nillable time fields

[htdvisser]

While trying out this project I think I found a bug in the generated code when using an optional and nillable time field.

vet: ent/user.go:46:16: cannot use &vu.DeletedAt (value of type *sql.NullTime) as *time.Time value in assignment

7438104 made a change to the {{ $scan }} struct, which now always uses {{ $f.NullType }} as type, so the $f.IsTime check can now be removed. This pull request does that.

This is my fist contribution here. I hope I didn't miss anything.

[a8m]

Thanks for your contribution @htdvisser.

In general, Optional means that the value is optional on entity creation and can be NULL in the database. If the value is NULL in the database, the struct field will hold a zero value of its type.

However, sometimes you want to distinguish between zero values and NULL (database column contains "", false or 0). Therefore, this is what Nillable option is used to. The struct field won't be nil only if the database column doesn't contain NULL.

Now, going back to the bug you found - I think we should address it. However, I'm not sure this is the way to go, since godoc for the time.Time package mentions that:

Programs using times should typically store and pass them as values, not pointers. That is, time variables and struct fields should be of type time.Time, not *time.Time.

The zero value of type Time is January 1, year 1, 00:00:00.000000000 UTC. As this time is unlikely to come up in practice, the IsZero method gives a simple way of detecting a time that has not been initialized explicitly.

My suggestion is to follow the Go standard and remove the Nillable option from the field.Time builder. But maybe I miss something. I would love to hear your thoughts, wdyt?

BTW, really impressive ramp-up. This reminds me that I need to write a better contribution doc.

[htdvisser]

For my DeletedAt I indeed decided to use only Optional and not use Nillable, which works fine so far.

However, I don't think that Nillable should be removed completely from field.Time. The way I read it, the "should typically" in the godoc implies that there may still be situations where it's acceptable to use *time.Time, and I think it would be good to let developers choose to do so.

[a8m]

and I think it would be good to let developers choose to do so.

Agree.

@alexsn any thoughts?

[facebook-github-bot]

@a8m has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[alexsn]

and I think it would be good to let developers choose to do so.

Agree.

@alexsn any thoughts?

I'm not sure I see a use case where you'd want to distinguish between not setting time and explicitly setting zero time. If there are such cases then Nillable should stay.

[htdvisser]

Thanks for accepting my pull request!

I think removing Nillable from field.Time would definitely have been out-of-scope for this particular pull request, but maybe it would be good to open an issue for discussion and make a decision on it before you release a v1.

[a8m]

Thanks for your contribution.