Unfiy time parsing with timestamp parsing

[msakrejda]

Fixes #251.

[johto]

I think using currentLocation for "time with time zone" is not an easily defendable behaviour. I'm inclined to think that timetz should always result in a time.FixedOffset.

[johto]

@deafbybeheading: Are you going to clean this up? I think we should commit a fix for the "time without time zone" issue.

[msakrejda]

Yeah, sorry--I thought this was a simple fix and then I realized the semantics we want are actually more nuanced. So if I understand correctly, those first four tests should instead be using time.FixedZone("", 0)?

[johto]

So if I understand correctly, those first four tests should instead be using time.FixedZone("", 0)?

First four? I don't understand where the number four comes from.

It looks like all uses of time.UTC in the tests are slightly incorrect, and barely don't fail because we only compare the offset (which is +00 for UTC, obviously) and not the location.

[msakrejda]

Sorry, meant first the tests added in the patch, starting here. Obviously there's more than four--I think something had scrolled off-screen and I miscounted. Should we be using time.FixedZone("", 0) everywhere in the tests then? And should we be using == instead of time.Equal?

[johto]

Sorry, meant first the tests added in the patch, starting here. Obviously there's more than four--I think something had scrolled off-screen and I miscounted. Should we be using time.FixedZone("", 0) everywhere in the tests then?

I think that would be more correct.

And should we be using == instead of time.Equal?

I think so. Especially in the location tests -- currently they're not testing much of anything, really. :-(

[msakrejda]

And should we be using == instead of time.Equal?

I think so. Especially in the location tests -- currently they're not testing much of anything, really. :-(

So we can't do that, because that compares the fields of the structs by value, including the pointer to the time.Location, which means if we instantiate two different zones with an identical name and offset, they'll still be treated as different. We can't really compare the time.Locationss manually, either, since the struct exposes no members, and the only function is String(), which is the same for time.Locations with different offsets but the same name.

I think we could use == of the respective time.String() values, however. Does that sound reasonable?

[johto]

So we can't do that, because that compares the fields of the structs by value, including the pointer to the time.Location, which means if we instantiate two different zones with an identical name and offset, they'll still be treated as different. We can't really compare the time.Locationss manually, either, since the struct exposes no members, and the only function is String(), which is the same for time.Locations with different offsets but the same name.

Ugh. Ugly.

I think we could use == of the respective time.String() values, however. Does that sound reasonable?

Given the above, I think that's the most reasonable thing to do.

[bendemaree]

Can I inquire on the status of this? I was bit by this and traced here from jinzhu/gorm#16. Let me know if I can lend a hand in some way.

[johto]

Can I inquire on the status of this? I was bit by this and traced here from jinzhu/gorm#16. Let me know if I can lend a hand in some way.

This only fixes problems with "time", not "timestamp with time zone".

I had a look, and it looks like the people in that issue are burned by the fact that negative time.Time values are not encoded correctly before they're sent into the database: https://gist.github.com/johto/d85b8a614e62b63fb31d. Personally I think that sending time.Time{} values is dubious at best, but the bug is probably still worth fixing.

[bendemaree]

Ah, I see @johto. I spent some time digging through this; hopefully this helps.

I think you are close, and that you may not have a negative time.Time bug at all. I am pretty sure the issue lies with sending a zeroed-out time.Time{}, as you suspected. In fact, when you run this through time.Format you get back 0001-01-01T00:00:00Z, which is reasonable. Then, asking Postgres to parse that is where it gets interesting.

testdb=# SELECT '0001-01-01T00:00:00Z'::timestamptz;
           timestamptz
---------------------------------
 0001-12-31 18:09:24-05:50:36 BC
(1 row)

The date is, I think, more accurately represented in Postgres, so it returns that value. I will talk to the maintainer of the other library about a cleaner way to represent a nil time.

[johto]

There's is a problem with negative years, and I've opened a pull request for that in #303.