gh-99392: Fix SQLite converter recipes

[naglis]

gh-99392: The converter functions are always passed a bytes object, datetime.date.fromisoformat and datetime.datetime.fromisoformat expect a str. In case of convert_timestamp() a cast to int is missing.

[erlend-aasland]

Thanks for catching that. Could you add doctests for this, so we're sure the CI catches it if we mess it up (again)?

[naglis]

@erlend-aasland I've added a doctest, not sure if it is any good.

Since the adapter for datetime.datetime is registered twice in the recipe (adapt_datetime_iso and adapt_datetime_epoch) I had to re-register them in each case so that the correct data is inserted. The re-registering does not feel nice.

You feedback would be appreciated.

[erlend-aasland]

I've added a doctest [...]

Thanks!

I think simple unit-tests of the actual functions should be sufficient. So something like this for each adapter/converter pair:

assert adapter_fn(val) == foo
assert converter_fn(val) == bar

Since the adapter for datetime.datetime is registered twice in the recipe (adapt_datetime_iso and adapt_datetime_epoch) I had to re-register them in each case so that the correct data is inserted. The re-registering does not feel nice.

Yes, perhaps the docs are a little unclear about this. Those are just two examples, or variants, of datetime.datetime adapter/converters. One where the database representation is a number (UNIX epoch), and one where it is stored as an ISO-8601 string. We could make that fact a little bit more explicit.

[naglis]

I've added a doctest [...]

Thanks!

I think simple unit-tests of the actual functions should be sufficient. So something like this for each adapter/converter pair:

assert adapter_fn(val) == foo
assert converter_fn(val) == bar

@erlend-aasland thank you, I've simplified the doctest. As fromtimestamp() returns the local date/time it is tricky to assert on a pre-defined timestamp, so I've used the timestamp of the current time. Is that ok or maybe there are better ways to handle this?

[erlend-aasland]

As fromtimestamp() returns the local date/time it is tricky to assert on a pre-defined timestamp, so I've used the timestamp of the current time. Is that ok or maybe there are better ways to handle this?

I'm fine with that. Thanks for adding the tests, and thanks again for the PR and the report!

[miss-islington]

Thanks @naglis for the PR, and @erlend-aasland for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒⛏🤖

[bedevere-bot]

GH-99419 is a backport of this pull request to the 3.11 branch.

[bedevere-bot]

GH-99420 is a backport of this pull request to the 3.10 branch.