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

FR: Consider using AsRef<Path> in SftpSession #7

Closed
netthier opened this issue Sep 27, 2023 · 3 comments
Closed

FR: Consider using AsRef<Path> in SftpSession #7

netthier opened this issue Sep 27, 2023 · 3 comments

Comments

@netthier
Copy link

Currently, all methods taking paths in russh_sftp::client::SftpSession take an impl Into<String>.
As these are actually all paths and my code treats these as paths, I find myself converting between strings and paths a lot.
I believe that it would make more sense to use AsRef<Path> for these methods instead, as this would also guarantee that the paths are valid on a type-level.

Also, the methods use both T and P for the generic path argument. Even if this request isn't implemented making those more consistent would be nice.

@AspectUnk AspectUnk added the enhancement New feature or request label Sep 28, 2023
@chipsenkbeil
Copy link

I'd actually argue that using paths might be misleading, at least if you use Rust's standard paths. This is because the path functionality you are using is associated with the client, not the server you are working with.

For instance, you are on Windows and connecting to an ssh server on Linux. Path and PathBuf will treat path components and operations as if they were/are operating on a Windows machine whereas the path the ssh server actually expects is for the Linux machine.

When you use something like serde to serialize/deserialize a path, it actually converts them to strings first, so I'd argue that you either keep strings or use something like my own typed-path that I made to address rust-lang/rust#66621.

In practicality, I don't think using Path or PathBuf as an argument will be an issue, but it definitely could be an issue when returning a path as you could imagine trying to convert C:\path\to\file.txt into a PathBuf on Linux could have unexpected consequences.

@netthier
Copy link
Author

netthier commented Sep 29, 2023

You're fully correct, I forgot that Path behavior is platform-specific.
I don't use serde to deserialize paths, but I am doing a lot of joining and getting parents in my SFTP code.
Thanks for the recommendation of typed-path; given the nature of that crate I don't think it'd be a good fit for this crate (since it's supposed to be remote-agnostic), but I'll definitely start using it in my code :v

EDIT: Just noticed that the newest version of that crate adds an enum type representing multiple possible path formats, maybe it could work after all 🤔

@chipsenkbeil
Copy link

You're fully correct, I forgot that Path behavior is platform-specific. I don't use serde to deserialize paths, but I am doing a lot of joining and getting parents in my SFTP code. Thanks for the recommendation of typed-path; given the nature of that crate I don't think it'd be a good fit for this crate (since it's supposed to be remote-agnostic), but I'll definitely start using it in my code :v

EDIT: Just noticed that the newest version of that crate adds an enum type representing multiple possible path formats, maybe it could work after all 🤔

Yeah, made that just now to make it a little easier to abstract. For now, your code would need to match on the type to do anything more specific (access UnixPath or WindowsPath), but I'll add in a wrapper to support the bulk of the operations directly from TypedPath in the near future.

@AspectUnk AspectUnk removed the enhancement New feature or request label Jul 16, 2024
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

No branches or pull requests

3 participants