Skip to content

Add UploadFileAsync and DownloadFileAsync methods #1634

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

Merged
merged 24 commits into from
May 5, 2025

Conversation

sdt-ndelarosa
Copy link
Contributor

@sdt-ndelarosa sdt-ndelarosa commented Apr 24, 2025

Adds TAP methods for UploadFile and DownloadFile to ISftpClient.

Other Solutions

While there are other options to define both the UploadFileAsync and DownloadFileAsync methods, such as the extension methods approach and the previously proposed pull request, #1515 , this pull request has the following benefits over the other solutions:

  • First class async/await support for uploading and downloading
    • Users no longer have to rely on implementing their own extension methods
  • Cleaner interop with the sync versions of the UploadFile and DownloadFile methods
    • Method signatures have a closer match
  • Upload and downloads now work with relative paths instead of just absolute paths
  • Deeper async/await call chain, as compared to BeginUploadFile, which pretty much runs the sync UploadFile method, but just in a different thread

@sdt-ndelarosa sdt-ndelarosa marked this pull request as ready for review April 24, 2025 16:59
@sdt-ndelarosa
Copy link
Contributor Author

actions don't seem to be triggering, gonna try closing and reopening

@Rob-Hague
Copy link
Collaborator

Hi, I will make time to review most likely at some point next week. FYI for first time contributors the CI needs to be run manually (some kind of github security measure)

Copy link
Collaborator

@Rob-Hague Rob-Hague left a comment

Choose a reason for hiding this comment

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

Looks good. I think it could do with testing cancellation - i.e. that it throws OperationCanceledException when the token is cancelled.

It would probably not be very reliable to test cancelling during a file download (at least not in an integration test), but we could at least have a test for when a cancelled token is supplied

@sdt-ndelarosa sdt-ndelarosa requested a review from Rob-Hague May 2, 2025 15:57
@sdt-ndelarosa sdt-ndelarosa requested a review from Rob-Hague May 5, 2025 16:30
Copy link
Collaborator

@Rob-Hague Rob-Hague left a comment

Choose a reason for hiding this comment

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

Thanks!

@Rob-Hague Rob-Hague merged commit d08c4aa into sshnet:develop May 5, 2025
4 checks passed
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.

2 participants