added ability for callbacks to have time.Time as arguments

[sum12]

go-sqlite3 can store time.Time values, and also has the fetaure to
register callback function to provide additionaly functions.

However these callback cannot take the mentioned time.Time columns as
input as there is no "dynamic-casting" of these values from TEXT to
time.Time.

With this patch that become possible letting user write custom functions
to do fancy filtering these time.Time columns

[sum12]

If the features is acceptable then if required I will add tests

[sum12]

The sqlite3_value is coming from the database. So can I assume it will not have timezone in it ?

[gjrtimmer]

https://www.sqlite.org/lang_datefunc.html

SQLite does support time strings with time zone.

See section Time Strings

[mattn]

please add test.

[sum12]

I have added tests. but I dont understand why the callbacks are failing for struct{}

[coveralls]

Coverage decreased (-0.6%) to 57.348% when pulling 3d52bcb on sum12:timecallback into 3aefd9f on mattn:master.

[coveralls]

Coverage decreased (-0.7%) to 57.657% when pulling 7791e15 on sum12:timecallback into 31f5bb8 on mattn:master.

[sum12]

okay that should do it. Let me know what you think

[sum12]

Any reviews on this one ?

[mattn]

Seems good to me. cc @gjrtimmer

[sum12]

thanks for fixing travis,
I have dropped the commit from the PR.

[sum12]

@gjrtimmer do you have any more reviews?

[mattn]

In recent changes, time.Time can be passed correctly. So IMO, this hack is not required. @sum12 Do you think?

[sum12]

Could you please point me to the commit that added this functionality? I looked at callback.go and it doesnot have it.

Or may be you could help me convert this hack to something that is not a hack ?

[rittneje]

@sum12 I do have a few comments on your changes.

1. Please use errors.New over fmt.Errorf if there are no format verbs (like %v).
2. Please remove "an" from the error message.
3. Please use C.GoStringN to account for embedded null bytes.
4. If the input fails to parse to a time.Time, you aren't actually returning an error. Is that the intended behavior? It seems inconsistent to complain about it not being TEXT, but to be fine with it not being an actual time string.
5. What if the input is an integer or float? It could be reasonable to interpret it as a Julian day or Unix timestamp. https://www.sqlite.org/lang_datefunc.html
6. What about blobs? Elsewhere this library tries to treat blobs and strings equivalently. (See callbackArgString.)

[sum12]

@sum12 I do have a few comments on your changes.

1. Please use `errors.New` over `fmt.Errorf` if there are no format verbs (like `%v`).

done

2. Please remove "an" from the error message.

hrm, done.

3. Please use `C.GoStringN` to account for embedded null bytes.

this one was good for learning, thanks.

4. If the input fails to parse to a `time.Time`, you aren't actually returning an error. Is that the intended behavior? It seems inconsistent to complain about it not being TEXT, but to be fine with it not being an actual time string.

make sense, corrected.

5. What if the input is an integer or float? It could be reasonable to interpret it as a Julian day or Unix timestamp. https://www.sqlite.org/lang_datefunc.html

the library does not recognize the other formats, only SQLiteTimestampFormats are recognized.
I would love to add support for other formats, but in a different PR, after this.

6. What about blobs? Elsewhere this library tries to treat blobs and strings equivalently. (See `callbackArgString`.)

Accepted and corrected

Thanks for reviews, however I am still learning Go. Do you see anything else not in line ?
may be @mattn or @gjrtimmer ?

[mattn]

@rittneje This is acceptable?

[rittneje]

You should just do reflect.TypeOf(time.Time{}). No need to take the hit of actually fetching the system times at startup.

[rittneje]

It is possible that the callback arg is not time.Time directly, but rather a typedef on it (e.g., type Foo time.Time). If we want to support such things, then instead you want to check if time.Time is convertible to typ (using the ConvertibleTo method).

On that note, it looks like there are actually bugs in the existing logic on this point. For instance, if I have a callback that takes a string typedef (e.g., type Bar string), that will be allowed, because the kind of that type is still reflect.String. But later the call will lead to a runtime panic. https://play.golang.org/p/MI-324y6veS

Fixing that will require appropriate calls to Convert. For example: https://play.golang.org/p/k77Y82MDNeL

The problem is even more interesting for []byte (aka []uint8), because you cannot actually convert between []byte and []TypedefOnByte. That particular case should probably be fixed to just check if the arg kind is not string and []byte is convertible to the arg type (or the arg type is specifically []byte) rather than doing a kind-based check.

To be clear, I am not suggesting you fix the existing bugs in this PR (although that would be very much appreciated). Just that we should decide whether such typedefs ought to be rejected - in which case your new code is fine as-is - or accepted - in which case we need the extra calls to ConvertibleTo and Convert. @mattn What are your thoughts on this point?