NXDRIVE-2967: Upgrade the deprecated datetime adapter #5313
Conversation
Reviewer's Guide by SourceryThis pull request upgrades the deprecated datetime adapter in the nxdrive project. The main changes involve modifying the execute method in the base.py file to handle datetime objects differently when passing parameters to SQL queries. Additionally, some warning suppressions related to the deprecated datetime adapter have been removed from the conftest.py file. No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## wip-NXDRIVE-2929-upgrade-python-from-3.9.5-to-3.12.3 #5313 +/- ##
========================================================================================
- Coverage 50.58% 47.80% -2.78%
========================================================================================
Files 96 96
Lines 16109 16121 +12
========================================================================================
- Hits 8148 7706 -442
- Misses 7961 8415 +454
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @gitofanindya - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider moving the datetime handling logic into a separate method to improve readability and maintainability of the execute() function.
- Avoid importing modules (datetime, sqlite3) inside the execute() method. Move these imports to the top of the file to prevent potential performance issues.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
nxdrive/dao/base.py
Outdated
| def adapt_datetime_iso(val): | ||
| return datetime.datetime.fromtimestamp(str(val), datetime.UTC) | ||
| mtime = sqlite3.register_adapter(param, adapt_datetime_iso) |
There was a problem hiding this comment.
issue: Improve datetime adaptation for SQLite
The current implementation of adapt_datetime_iso is incorrect and the adapter registration is inefficient. Consider using isoformat() for datetime conversion and register the adapter once outside the loop. For example:
def adapt_datetime_iso(val):
return val.isoformat()
sqlite3.register_adapter(datetime.datetime, adapt_datetime_iso)
In the loop
if isinstance(param, datetime.datetime):
new_param.append(param) # SQLite will use the registered adapter
nxdrive/dao/base.py
Outdated
| new_param = [] | ||
| for param in parameters: | ||
| if type(param) == datetime.datetime: | ||
| def adapt_datetime_iso(val): | ||
| return datetime.datetime.fromtimestamp(str(val), datetime.UTC) | ||
| mtime = sqlite3.register_adapter(param, adapt_datetime_iso) | ||
| new_param.append(mtime) | ||
| else: | ||
| new_param.append(param) | ||
|
|
||
| new_param = tuple(new_param) |
There was a problem hiding this comment.
suggestion (performance): Optimize parameter processing
Instead of creating a list and then converting it to a tuple, consider using a generator expression for better performance. For example:
new_param = tuple(adapt_datetime_iso(param) if isinstance(param, datetime.datetime) else param for param in parameters)
def adapt_datetime_iso(val):
return datetime.datetime.fromtimestamp(str(val), datetime.UTC)
sqlite3.register_adapter(datetime.datetime, adapt_datetime_iso)
new_param = tuple(
adapt_datetime_iso(param) if isinstance(param, datetime.datetime) else param
for param in parameters
)
| elif "The default datetime adapter is deprecated as of Python 3.12" in message: | ||
| continue |
There was a problem hiding this comment.
suggestion (testing): Removal of warning suppression for deprecated datetime adapter
The removal of this warning suppression suggests that the issue with the deprecated datetime adapter has been addressed. However, it would be beneficial to add a test that verifies the new datetime handling in the execute method of nxdrive/dao/base.py.
@pytest.fixture(autouse=True)
def ignore_warnings(request):
warnings.filterwarnings("ignore", category=DeprecationWarning)
warnings.filterwarnings("ignore", category=ResourceWarning)
yield
warnings.resetwarnings()
def test_datetime_handling():
from nxdrive.dao.base import execute
# Add test logic here to verify datetime handling
Summary by Sourcery
Upgrade the handling of datetime objects in SQL execution to address deprecation warnings by implementing a custom adapter, and update test configurations to reflect this change.
Bug Fixes:
Tests: