Skip to content
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

Make SFTPFile.seek return the new position and .write return the number of written bytes #1696

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mxmlnkn
Copy link
Contributor

@mxmlnkn mxmlnkn commented Sep 29, 2024

Fixes #1695

@@ -13,6 +13,35 @@
logger = logging.getLogger("fsspec.sftp")


def _patch_SFTPFile(file):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hah, ugly :( (not a criticism of your work, just a shame)
Does this create reference cycles on self and the closure?

Other options might be:

  • patch the upstream ftp.open to return a subclass FsspecSShFile(SFTPFile, IOBase)
  • make our own class with the upstream file object as an attribute and pass all the methods through except for these two

Do these sound more palatable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I kinda agree that it is not the most beautiful. I also considered a separate file object class, but I didn't want to copy 10+ methods with the danger of missing some. Subclassing also does not work because the object is created inside SFTPClient.open.

I have also opened a PR to fix this upstream. I guess it is fine to wait for that to be merged and simply close this PR and issue here. I might have been too impatient. Fixing this here has the advantage that it will work with older versions of paramiko just fine, though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I'm glad you made that PR. In that case let's wait - it doesn't appear that this part of the API gets touched much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

File object opened via SSH is inconsistent with the file object API. Seek and write return nothing
2 participants