Allow empty location as no location info

[nasermirzaei89]

Package github.com/lib/pq converts Postgres type "Date" to time.Time with zero value location, not nil.
So, scanning it by date.Date returns error.

However I believe this is an issue on github.com/lib/pq, we can add this patch to date.Date.

Regarding the nil location, golang itself already handles this and returns UTC.

[dhermes]

@nasermirzaei89 Do you feel equipped to add a unit test for this case? (I'm fine with the change but want to make sure we have a test that FAILS without this change but passes with it.)

[nasermirzaei89]

OK, I'll add the test.

[dhermes]

Given the naming, wouldn't it make sense for this to be

hasLocationInfo := t.Location() == time.UTC || t.Location().String() == ""

and then use !hasLocationInfo on line 124?

[nasermirzaei89]

and then the naming doesn't make sense.
I can make it inline and remove the variable. Shall I do that?

[dhermes]

Oh I was thinking about it wrong, I get it now!

[dhermes]

I think the failing test showcases an issue with this approach

-timestamp contains more than just date information; 2022-01-31T00:00:00-05:00
+<nil>

In particular, the test case ends up resulting in a time.FixedZone("", -5*60*60) which isn't a "zero offset" even though it has a name equal to "". Note that time.Location{}.String() just returns the name and does not mention the offset.

---

Do you have an example value deserialized by lib/pq? Could you fmt.Printf("%#v\n", tz) the value and peek at the internal fields?

[dhermes]

Actually t.Zone() may do the trick for us here

[dhermes]

@nasermirzaei89 Testing this is quirkier than I realized! I starting experimenting with time.Time{}.Zone() in my local checkout and it started to become a big thing!

I'm sending a PR with test-only changes, then your feature will be easier to test and add tests for.

[dhermes]

@nasermirzaei89 #13 will merge shortly and then you can merge it in / rebase on top of it

[nasermirzaei89]

@dhermes as the input for the function is time.Time, this field shouldn't be time.Time rather than string?
https://github.com/hardfinhq/go-date/blob/main/convert_test.go#L78

[dhermes]

@nasermirzaei89 The input is intentionally string so the test author can just write a valid RFC 3339 string and then the test code will handle parsing it into a time.Time{}.

But makes it impossible to handle named timezones, so I added that in #13.

[nasermirzaei89]

@dhermes could you please take a look at my second commit in this pull request and if it's not the desired solution, I can revert it and continue with the #13 approach.

[dhermes]

@nasermirzaei89 So sorry, I wasn't intending for you to refactor the existing tests! Definitely not the direction I want to take that test in. Sorry we didn't have a chance to discuss before you put that time in.

As for your code itself, you'll need to tweak it to using t.Zone() and I think just checking that the offset is equal to 0 should be sufficient.

[nasermirzaei89]

@dhermes If I get #13 correctly, it has a timezone that translates the time to its location. So when we translate a time with time zone -0500 by -21600, it goes to timezone zero or UTC. So it converts to Date without error.

[dhermes]

So when we translate a time with time zone -0500 by -21600, it goes to timezone zero or UTC. So it converts to Date without error.

The issue is that it's a timezone with no name, so the t.Location().String() check is a false positive. Instead you should be using _, offset = t.Zone() and checking offset = 0.

[nasermirzaei89]

I've updated the code to use zone and offset. However, since TestFromTime makes some assumptions beyond the FromTime itself, I can't precisely test the lib/pq case.

[dhermes]

However, since TestFromTime makes some assumptions beyond the FromTime itself, I can't precisely test the lib/pq case.

All good, I can sort out how to test from here!