-
Notifications
You must be signed in to change notification settings - Fork 53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
NXDRIVE-2967: Upgrade the deprecated datetime adapter #5312
NXDRIVE-2967: Upgrade the deprecated datetime adapter #5312
Conversation
…NXDRIVE-2929-upgrade-python-from-3.9.5-to-3.12.3
…NXDRIVE-2929-upgrade-python-from-3.9.5-to-3.12.3
…NXDRIVE-2929-upgrade-python-from-3.9.5-to-3.12.3
Reviewer's Guide by SourceryThis pull request upgrades the deprecated datetime adapter and updates the Python version from 3.9 to 3.12 across the project. The changes primarily focus on updating datetime usage to be timezone-aware and modifying the Python version in various configuration files and documentation. Class diagram for updated datetime handlingclassDiagram
class BaseDAO {
+execute(sql: str, parameters: Iterable[Any]) Cursor
}
class Engine {
+increase_error(row: DocPair, error: str, details: str, incr: int)
+unsynchronize_state(row: DocPair)
+unset_unsychronised(row: DocPair)
+synchronize_state(row: DocPair)
}
class RemoteWatcher {
+scan_remote(from_state: DocPair)
}
BaseDAO --> Engine : uses
Engine --> RemoteWatcher : uses
note for BaseDAO "Updated execute method to handle timezone-aware datetime"
note for Engine "Updated methods to use timezone-aware datetime"
note for RemoteWatcher "Updated scan_remote to use timezone-aware datetime"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
🔍 Existing Issues For ReviewYour pull request is modifying functions with the following pre-existing issues: 📄 File: nxdrive/engine/engine.py
Did you find this useful? React with a 👍 or 👎 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @gitofanindya - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider refactoring the
execute
method innxdrive/dao/base.py
to improve readability. The added complexity for datetime handling could be extracted into a separate function. - Excellent job on consistently updating the Python version across all CI workflows and documentation. This thorough approach helps maintain consistency throughout the project.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟡 Documentation: 1 issue found
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
@@ -25,7 +25,24 @@ def execute(self, sql: str, parameters: Iterable[Any] = ()) -> Cursor: | |||
while True: | |||
count += 1 | |||
try: | |||
return super().execute(sql, parameters) | |||
if parameters: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (performance): Consider moving imports outside the execute method
Moving the imports (datetime and sqlite3) to the top of the file could improve performance, especially if this method is called frequently.
import datetime
import sqlite3
class BaseDAO:
def execute(self, sql, parameters=None):
count = 0
while True:
count += 1
try:
if parameters:
As of now, we are using the __Python 3.12.3__. | ||
|
||
History: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (documentation): Consider updating the History section if necessary.
If there's a history of Python versions used, it might be helpful to add this latest version to that list.
As of now, we are using the __Python 3.12.3__. | |
History: | |
As of now, we are using __Python 3.12.3__. | |
History: | |
- Python 3.12.3 (current) | |
- Python 3.9.5 (previous) |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5312 +/- ##
==========================================
+ Coverage 49.24% 50.61% +1.37%
==========================================
Files 94 96 +2
Lines 15699 16123 +424
==========================================
+ Hits 7731 8161 +430
+ Misses 7968 7962 -6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Summary by Sourcery
Upgrade the datetime handling to use timezone-aware
datetime.now(tz=timezone.utc)
instead ofdatetime.utcnow()
. Update the Python version to 3.12 across all CI workflows and documentation.Enhancements:
datetime.utcnow()
withdatetime.now(tz=timezone.utc)
for better timezone handling.locale.getlocale()
instead oflocale.getdefaultlocale()
.CI:
Documentation: