Fix DeprecationWarning on 3.12 with a datetime adapter for sqlite3 #14139
Conversation
IPython/core/history.py
Outdated
| if sys.version_info >= (3, 12): | ||
|
|
||
| def _adapt_datetime(val): | ||
| return val.isoformat(" ") | ||
|
|
||
| sqlite3.register_adapter(datetime.datetime, _adapt_datetime) |
There was a problem hiding this comment.
Do you believe it would be safer to not register adapter and call isoformat(" ") on the actual places where we insert data into sqlite ?
The fact that the upstream issue tell us there are issues with the default adapters is maybe a hint we should not re-register the same as it has global effects.
In the long term, I'm also wondering why isoformat(" "), and not just isoformat(), and if there would be any way we could migrate to just isoformat()...
There was a problem hiding this comment.
Do you believe it would be safer to not register adapter and call isoformat(" ") on the actual places where we insert data into sqlite ?
I'm not sure. I hit this warning while running pytest diofant/tests/interactive for the diofant project and it seems to be related to the statement in the HistoryManager.new_session() method.
But maybe there are more such cases... The default adapter
The fact that the upstream issue tell us there are issues with the default adapters
The default adapters now are explicit rather than implicit. This is the only difference. I don't think this means there is an issue with the notion of default adapters or something like that...
I'm also wondering why isoformat(" "), and not just isoformat(), and if there would be any way we could migrate to just isoformat()
That's a literal copy of the current default adapter from the sqlite3 module. " " is because the default value for sep kwarg is "T", see docs.
There was a problem hiding this comment.
he default adapters now are explicit rather than implicit. This is the only difference. I don't think this means there is an issue with the notion of default adapters or something like that...
I was more refering to the text in python/cpython#90016 saying "the default converters in SQLite3 have several bugs"
That's a literal copy of the current default adapter from the sqlite3 module. " " is because the default value for
sepkwarg is "T", see docs.
Yes, I got that as well, I'm wondering if we should use the default with T, obviously we want to be careful as we don't want to mess up existing history.
There was a problem hiding this comment.
I was more refering to the text in python/cpython#90016 saying "the default converters in SQLite3 have several bugs"
The problem is that in the alternative fix you are using same default adapter, but reinvent this code in several places instead. Default adapters are exactly for this type of things...
Yes, I got that as well, I'm wondering if we should use the default with T
Perhaps, but this will change the db format. So, probably this will require a deprecation period, etc.
There was a problem hiding this comment.
Yes, but I am using the fix only in IPython internal code. If we reinject the same default adapter with sqlite3.register_adapter this mean we affect code that runs in IPython. Code that will work in IPython may not work in pure Python, in particular is code that use sql and datetime. Once CPython remove the default adapter this code will work in IPython but fail in CPython
|
I'll try an alternative where I call .isoformat(" ") explicitely were necessary and don't change the default adapter. |
After python/cpython#90016, default adapters and converters are deprecated. See also https://docs.python.org/3.12/library/sqlite3.html#sqlite3-adapter-converter-recipes
I think we need to avoid registering a default adapter, especially if
CPython want to remove them.
So we are going to call .isoformat(" ") explicitly. For backward compat
we'll keep the delimiter as " ", but we might want to consider using the
default of T later on, but still keeping in mind that old history have a
space.
|
Thanks ! |
After python/cpython#90016, default adapters and converters are deprecated. See also
https://docs.python.org/3.12/library/sqlite3.html#sqlite3-adapter-converter-recipes