NXDRIVE-2967: Upgrade the deprecated datetime adapter.. #5445
Conversation
Reviewer's Guide by SourceryThis PR updates several dependencies to their latest versions and fixes timezone-related issues in datetime handling. The main changes include upgrading Python packages, addressing security vulnerabilities, and implementing a fix for zero-byte file handling. Sequence diagram for zero-byte file handlingsequenceDiagram
participant FoldersDialog
participant Path
FoldersDialog->>Path: get_size(file_path)
Path-->>FoldersDialog: file_size
alt file_size == 0
FoldersDialog->>FoldersDialog: Ignore zero-byte file
else file_size > 0
FoldersDialog->>FoldersDialog: Add file to paths
end
Class diagram for updated datetime handlingclassDiagram
class AutoRetryCursor {
+adapt_datetime_iso(val)
+execute(sql: str, parameters: Iterable[Any])
}
note for AutoRetryCursor "Updated to handle datetime with timezone awareness using adapt_datetime_iso method"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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 and they look great!
Here's what I looked at during the review
- 🟡 General issues: 5 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 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.
| def execute(self, sql: str, parameters: Iterable[Any] = ()) -> Cursor: | ||
| count = 1 | ||
| while True: | ||
| count += 1 | ||
| try: | ||
| return super().execute(sql, parameters) | ||
| import sqlite3 | ||
| # return super().execute(sql, parameters) |
There was a problem hiding this comment.
issue: The datetime parameter conversion logic should be simplified and the original super().execute() call restored
The current implementation with register_adapter could have unintended side effects. Consider moving the datetime conversion to a separate method and maintaining the original execution flow.
nxdrive/gui/folders_dialog.py
Outdated
| @@ -624,6 +624,9 @@ def _files_display(self) -> str: | |||
| txt += f" (+{self.overall_count - 1:,})" | |||
| return txt | |||
|
|
|||
| def get_size(self, file_path: Path) -> bool: | |||
There was a problem hiding this comment.
issue (bug_risk): Return type hint is incorrect - method returns an integer not a boolean
nxdrive/gui/folders_dialog.py
Outdated
| @@ -647,10 +650,17 @@ def _process_additionnal_local_paths(self, paths: List[str], /) -> None: | |||
| # Save the path | |||
| if path.is_dir(): | |||
| for file_path, size in get_tree_list(path): | |||
| if self.get_size(file_path) == 0: | |||
There was a problem hiding this comment.
suggestion (performance): Redundant size check - file size is already available from get_tree_list()
Consider using the size value already provided by get_tree_list() instead of calling get_size() again.
| if self.get_size(file_path) == 0: | |
| if size == 0: |
| except (ValueError, OverflowError, OSError) as e: | ||
| log.warning( | ||
| f"{e} file path: {os_path}. st_mtime value: {stat_info.st_mtime}" | ||
| ) | ||
| mtime = datetime.utcfromtimestamp(0) | ||
| mtime = datetime.fromtimestamp(0, tz=timezone.utc) |
There was a problem hiding this comment.
suggestion: Error handling should use consistent timezone approach
Both the try and except blocks now use timezone-aware timestamps, but the error handling could be more consistent by using a common approach or constant for the fallback time.
| mtime = datetime.fromtimestamp(0, tz=timezone.utc) | |
| EPOCH = datetime(1970, 1, 1, tzinfo=timezone.utc) | |
| mtime = EPOCH |
nxdrive/engine/engine.py
Outdated
| else self.dao.get_dt_upload | ||
| if is_direct_transfer | ||
| else self.dao.get_upload | ||
| else self.dao.get_dt_upload if is_direct_transfer else self.dao.get_upload |
There was a problem hiding this comment.
suggestion: Nested ternary operator could be simplified for better readability
Consider using an if/elif/else block to make the logic more explicit and easier to maintain.
else:
if is_direct_transfer:
meth = self.dao.get_dt_upload
else:
meth = self.dao.get_upload
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 #5445 +/- ##
========================================================================================
+ Coverage 50.57% 52.06% +1.48%
========================================================================================
Files 96 96
Lines 16116 16123 +7
========================================================================================
+ Hits 8151 8394 +243
+ Misses 7965 7729 -236
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@dependabot rebase |
…https://github.com/nuxeo/nuxeo-drive into test_datetime_adapter_new
Summary by Sourcery
Upgrade dependencies and improve datetime handling. Update the macOS release workflow to use a .p12 certificate format. Remove deprecated datetime adapter warnings from tests.
Enhancements:
CI:
Documentation:
Tests: