-
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
Test dt adapter #5524
base: wip-NXDRIVE-2929-upgrade-python-from-3.9.5-to-3.12.3
Are you sure you want to change the base?
Test dt adapter #5524
Conversation
* Bump platformdirs from 4.2.2 to 4.3.6 in /tools/deps Bumps [platformdirs](https://github.com/tox-dev/platformdirs) from 4.2.2 to 4.3.6. - [Release notes](https://github.com/tox-dev/platformdirs/releases) - [Changelog](https://github.com/tox-dev/platformdirs/blob/main/CHANGES.rst) - [Commits](tox-dev/platformdirs@4.2.2...4.3.6) --- updated-dependencies: - dependency-name: platformdirs dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> * Bump faker from 22.0.0 to 29.0.0 in /tools/deps (#5271) Bumps [faker](https://github.com/joke2k/faker) from 22.0.0 to 29.0.0. * Bump build from 1.2.1 to 1.2.2 in /tools/deps (#5274) Bumps [build](https://github.com/pypa/build) from 1.2.1 to 1.2.2. --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Pooja Ramkrishna Daine <[email protected]>
* Bump watchdog from 3.0.0 to 5.0.2 in /tools/deps Bumps [watchdog](https://github.com/gorakhargosh/watchdog) from 3.0.0 to 5.0.2. - [Release notes](https://github.com/gorakhargosh/watchdog/releases) - [Changelog](https://github.com/gorakhargosh/watchdog/blob/master/changelog.rst) - [Commits](gorakhargosh/watchdog@v3.0.0...v5.0.2) --- updated-dependencies: - dependency-name: watchdog dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> * Bump mypy from 1.10.0 to 1.11.2 in /tools/deps (#5207) Bumps [mypy](https://github.com/python/mypy) from 1.10.0 to 1.11.2. * Bump responses from 0.24.1 to 0.25.3 in /tools/deps (#5006) Bumps [responses](https://github.com/getsentry/responses) from 0.24.1 to 0.25.3. * Bump python-dateutil from 2.8.2 to 2.9.0.post0 in /tools/deps (#4680) Bumps [python-dateutil](https://github.com/dateutil/dateutil) from 2.8.2 to 2.9.0.post0. * Bump types-python-dateutil in /tools/deps (#5230) Bumps [types-python-dateutil](https://github.com/python/typeshed) from 2.8.19.20240106 to 2.9.0.20240906. --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Pooja Ramkrishna Daine <[email protected]>
* Bump pyobjc-framework-cocoa from 10.1 to 10.3.1 in /tools/deps Bumps [pyobjc-framework-cocoa](https://github.com/ronaldoussoren/pyobjc) from 10.1 to 10.3.1. - [Release notes](https://github.com/ronaldoussoren/pyobjc/releases) - [Changelog](https://github.com/ronaldoussoren/pyobjc/blob/master/docs/changelog.rst) - [Commits](ronaldoussoren/pyobjc@v10.1...v10.3.1) --- updated-dependencies: - dependency-name: pyobjc-framework-cocoa dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> * Bump pyobjc-framework-scriptingbridge from 10.1 to 10.3.1 in /tools/deps (#4987) Bumps [pyobjc-framework-scriptingbridge](https://github.com/ronaldoussoren/pyobjc) from 10.1 to 10.3.1. * Bump pyobjc-core from 10.1 to 10.3.1 in /tools/deps (#4990) Bumps [pyobjc-core](https://github.com/ronaldoussoren/pyobjc) from 10.1 to 10.3.1. --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Pooja Ramkrishna Daine <[email protected]>
) * Bump pyobjc-framework-fsevents from 10.1 to 10.3.1 in /tools/deps Bumps [pyobjc-framework-fsevents](https://github.com/ronaldoussoren/pyobjc) from 10.1 to 10.3.1. - [Release notes](https://github.com/ronaldoussoren/pyobjc/releases) - [Changelog](https://github.com/ronaldoussoren/pyobjc/blob/master/docs/changelog.rst) - [Commits](ronaldoussoren/pyobjc@v10.1...v10.3.1) --- updated-dependencies: - dependency-name: pyobjc-framework-fsevents dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> * Bump pyobjc-framework-coreservices from 10.1 to 10.3.1 in /tools/deps (#4986) Bumps [pyobjc-framework-coreservices](https://github.com/ronaldoussoren/pyobjc) from 10.1 to 10.3.1. * Bump pyobjc-framework-systemconfiguration in /tools/deps (#4989) Bumps [pyobjc-framework-systemconfiguration](https://github.com/ronaldoussoren/pyobjc) from 10.1 to 10.3.1. --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Pooja Ramkrishna Daine <[email protected]>
* Update deploy_ci_agent.sh
* Bump send2trash from 1.7.1 to 1.8.3 in /tools/deps Bumps [send2trash](https://github.com/arsenetar/send2trash) from 1.7.1 to 1.8.3. - [Release notes](https://github.com/arsenetar/send2trash/releases) - [Changelog](https://github.com/arsenetar/send2trash/blob/master/CHANGES.rst) - [Commits](https://github.com/arsenetar/send2trash/commits) --- updated-dependencies: - dependency-name: send2trash dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> * Bump altgraph from 0.17 to 0.17.4 in /tools/deps (#4565) Bumps [altgraph](https://github.com/ronaldoussoren/altgraph) from 0.17 to 0.17.4. --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Pooja Ramkrishna Daine <[email protected]>
* Bump requests from 2.31.0 to 2.32.3 in /tools/deps Bumps [requests](https://github.com/psf/requests) from 2.31.0 to 2.32.3. - [Release notes](https://github.com/psf/requests/releases) - [Changelog](https://github.com/psf/requests/blob/main/HISTORY.md) - [Commits](psf/requests@v2.31.0...v2.32.3) --- updated-dependencies: - dependency-name: requests dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> * Bump future from 0.18.3 to 1.0.0 in /tools/deps (#4693) Bumps [future](https://github.com/PythonCharmers/python-future) from 0.18.3 to 1.0.0. * Bump pefile from 2023.2.7 to 2024.8.26 in /tools/deps (#5191) Bumps [pefile](https://github.com/erocarrera/pefile) from 2023.2.7 to 2024.8.26. * Bump pycparser from 2.21 to 2.22 in /tools/deps (#4753) Bumps [pycparser](https://github.com/eliben/pycparser) from 2.21 to 2.22. --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Pooja Ramkrishna Daine <[email protected]>
* NXDRIVE-2925: Ignore zero-byte files
* Bump black from 23.12.1 to 24.10.0 in /tools/deps Bumps [black](https://github.com/psf/black) from 23.12.1 to 24.10.0. - [Release notes](https://github.com/psf/black/releases) - [Changelog](https://github.com/psf/black/blob/main/CHANGES.md) - [Commits](psf/black@23.12.1...24.10.0) --- updated-dependencies: - dependency-name: black dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Pooja Ramkrishna Daine <[email protected]> Co-authored-by: unknown <[email protected]>
…st_datetime_adapter_new
Reviewer's Guide by SourceryThis pull request upgrades the Python version to 3.12, updates dependencies, and modifies datetime handling to use timezone-aware datetime objects. It also includes documentation updates and a fix for ignoring zero-byte files. Sequence diagram for timezone-aware datetime handlingsequenceDiagram
participant LC as LocalClient
participant DB as SQLite Database
participant AC as AutoRetryCursor
LC->>DB: Get file info
activate LC
Note over LC: Convert file mtime
LC->>LC: datetime.fromtimestamp(st_mtime, tz=timezone.utc)
LC-->>DB: Store timezone-aware datetime
deactivate LC
DB->>AC: Execute SQL query
activate AC
Note over AC: Adapt datetime for SQLite
AC->>AC: adapt_datetime_iso()
AC-->>DB: Store UTC datetime
deactivate AC
Class diagram for AutoRetryCursor enhancementclassDiagram
class AutoRetryCursor {
+adapt_datetime_iso(val)
+execute(sql: str, parameters: Iterable[Any])
}
class Cursor {
+execute()
}
AutoRetryCursor --|> Cursor
note for AutoRetryCursor "New timezone handling"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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:
- The sqlite datetime adapter implementation in AutoRetryCursor needs cleanup - remove commented code and avoid direct sqlite3 imports. Consider using the standard sqlite3 date adapters.
- The removal of MockedPath test class reduces test coverage. Please either restore the test coverage or document why these tests are no longer needed.
- Using os.system() for directory removal is unsafe and platform-dependent. Please use pathlib or shutil functions instead.
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 2 issues 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.
class AutoRetryCursor(Cursor): | ||
def adapt_datetime_iso(self, val): |
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.
issue: The strftime('%s') format specifier is not guaranteed to work on all platforms, particularly Windows
con = sqlite3.connect(str(database)) | ||
with dump_file.open(mode="w", encoding="utf-8") as f: | ||
for line in con.iterdump(): | ||
f.write(f"{line}\n") | ||
|
||
# Force write of file to disk | ||
f.flush() | ||
fsync(f.fileno()) | ||
con.close() |
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: Use context manager pattern for database connection to ensure proper cleanup
This ensures the connection is closed even if an exception occurs during the dump process.
con = sqlite3.connect(str(database)) | |
with dump_file.open(mode="w", encoding="utf-8") as f: | |
for line in con.iterdump(): | |
f.write(f"{line}\n") | |
# Force write of file to disk | |
f.flush() | |
fsync(f.fileno()) | |
con.close() | |
with sqlite3.connect(str(database)) as con, dump_file.open(mode="w", encoding="utf-8") as f: | |
for line in con.iterdump(): | |
f.write(f"{line}\n") | |
# Force write of file to disk | |
f.flush() | |
fsync(f.fileno()) |
nxdrive/utils.py
Outdated
@@ -1315,7 +1315,7 @@ def get_current_locale() -> str: | |||
l10n_code = NSLocale.currentLocale() | |||
l10n = NSLocale.localeIdentifier(l10n_code) | |||
else: | |||
l10n = locale.getdefaultlocale()[0] or "" | |||
l10n = locale.getlocale()[1] 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.
issue (bug_risk): Incorrect index used for locale code - should be [0] instead of [1]
Index [1] returns the encoding instead of the locale code. This will cause incorrect locale detection.
@@ -40,7 +40,8 @@ def __enter__(self): | |||
|
|||
def __exit__(self, exc_type, exc_val, exc_tb): | |||
self.dispose() | |||
self.db.unlink() |
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.
issue (testing): Using os.system() for test cleanup is unsafe
Using os.system() for file operations in tests is not recommended as it's platform-dependent and can be unsafe. Consider using pathlib.Path.unlink() or shutil.rmtree() for more reliable cleanup.
db.unlink() | ||
os.system(f'rmdir "{db}"') | ||
|
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.
issue (testing): Same unsafe cleanup pattern in test_proxy.py
Similar to the issue in conftest.py, using os.system() for cleanup is not recommended. Use standard Python file operations instead.
@sourcery-ai review |
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:
- The use of
rmdir
instead ofunlink()
in the tests seems potentially problematic. Consider keeping the originalunlink()
call or properly handling non-empty directory cases ifrmdir
is really needed.
Here's what I looked at during the review
- 🟡 General issues: 3 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.
# return super().execute(sql, parameters) | ||
# new_param = tuple( datetime.fromtimestamp(param, tz=timezone.utc) if isinstance(param, datetime) else param for param in parameters ) | ||
new_param = tuple( sqlite3.register_adapter(param, self.adapt_datetime_iso) if isinstance(param, datetime) else param for param in 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.
issue (bug_risk): The datetime adapter registration approach could cause issues with concurrent database access
Using register_adapter affects the entire SQLite connection. Consider using value adaptation at the parameter level instead. Also, the original code is still present in comments which should be cleaned up.
if self.get_size(file_path) == 0: | ||
# ignoring zero byte files [NXDRIVE-2925] | ||
continue | ||
self.paths[file_path] = size | ||
else: | ||
try: | ||
self.paths[path] = path.stat().st_size | ||
file_size = self.get_size(path) | ||
if file_size == 0: | ||
# ignoring zero byte files [NXDRIVE-2925] | ||
continue |
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: Consider extracting the zero-byte file check into a helper method to avoid code duplication
The zero-byte file check logic is repeated. Consider creating a helper method like _should_skip_file(path) to handle this check in both cases.
Suggested implementation:
def get_size(self, file_path: Path) -> bool:
return file_path.stat().st_size
def _should_skip_file(self, file_path: Path) -> bool:
"""Check if file should be skipped (zero-byte files) [NXDRIVE-2925]."""
return self.get_size(file_path) == 0
if self._should_skip_file(file_path):
continue
file_size = self.get_size(path)
if self._should_skip_file(path):
continue
self.paths[path] = file_size
con = sqlite3.connect(str(database)) | ||
with dump_file.open(mode="w", encoding="utf-8") as f: | ||
for line in con.iterdump(): | ||
f.write(f"{line}\n") | ||
|
||
# Force write of file to disk | ||
f.flush() | ||
fsync(f.fileno()) | ||
con.close() |
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: Use a context manager for database connection to ensure proper resource cleanup
While the connection is being closed explicitly, using a context manager (with statement) would be safer and ensure the connection is closed even if an exception occurs.
Summary by Sourcery
Update dependencies to fix security issues and renew MacOS certificate. Ignore zero-byte files during upload.
New Features:
Bug Fixes:
Tests: