russh-sftp

russh-sftp copied to clipboard

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.

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 https://github.com/rust-lang/rust/issues/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.

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 :thinking:

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.