fix: replace deprecated datetime.utcfromtimestamp#70
Conversation
datetime.utcfromtimestamp is deprecated and slated for removal. Route both timestamps through a _naive_utc helper that uses datetime.fromtimestamp(ts, timezone.utc) and strips tzinfo, preserving the existing naive-UTC values returned in info dicts.
There was a problem hiding this comment.
Pull request overview
Updates timestamp handling in SSHFileSystem to avoid the deprecated datetime.utcfromtimestamp() while preserving the existing behavior of returning naive UTC datetime objects in info() dictionaries.
Changes:
- Import
timezoneand introduce a private_naive_utc()helper that converts POSIX timestamps viadatetime.fromtimestamp(..., timezone.utc)and stripstzinfo. - Route SFTP attribute
atime/mtimedecoding through_naive_utc()to maintain prior naive-UTC outputs.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
shcheklein
left a comment
There was a problem hiding this comment.
Thanks for the PR!
- We need tests for this
- Let's review and do a change that is aligned with fsspec.implementations.sftp and LocalFileSystem. I think they do
tz-aware UTC datetimesfromtimestamp(ts, tz=timezone.utc - Helper probably should disappear
- Pre-existing:
SFTPAttrs.atime/mtime are Optional- worth fixing it here (don't try to convert - just return None)
|
@mixilchenko thanks! could you please update the PR description? |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
sshfs/spec.py:158
- The PR description says the goal is to preserve the existing naive UTC semantics of
datetime.utcfromtimestamp(...)by strippingtzinfo, but the new implementation returns tz-aware datetimes (tz=timezone.utc). That’s a behavior/API change forinfo()/ls(detail=True)and may break callers which expect naive UTC datetimes.
Consider routing both timestamps through a small helper which returns naive-UTC (i.e., fromtimestamp(..., tz=UTC) then replace(tzinfo=None)), matching the PR’s stated intent while also handling None safely.
"time": (
datetime.fromtimestamp(attributes.atime, tz=timezone.utc)
if attributes.atime is not None
else None
),
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #70 +/- ##
==========================================
- Coverage 94.30% 93.41% -0.89%
==========================================
Files 13 12 -1
Lines 738 775 +37
==========================================
+ Hits 696 724 +28
- Misses 42 51 +9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
datetime.utcfromtimestamp()is deprecated since Python 3.12 and slated for removal.SSHFileSystem._decode_attributes()used it to decode SFTPatime/mtime, so everyinfo()/ls(detail=True)/modified()call emits aDeprecationWarningon modern Python and will break outright once the function is removed.This PR replaces it with
datetime.fromtimestamp(ts, tz=timezone.utc).Behavior changes (⚠️ for downstream consumers):
info()["time"],info()["mtime"]andmodified()now return timezone-aware UTC datetimes instead of naive ones. The instant represented is identical; onlytzinfochanged. This aligns sshfs withfsspec's ownLocalFileSystemandsftpimplementations. Code that compares these values against naive datetimes will now raiseTypeErrorand should switch to aware datetimes (e.g.datetime.now(timezone.utc)).atime/mtimemissing from the server response now yieldNoneinstead of raisingTypeError(SFTPAttrsfields are optional).Tests:
info()timestamps are asserted to be tz-aware UTC, locking in the new contract._decode_attributes()withSFTPAttrs(atime=None, mtime=None)returnsNonefor both fields.