Custom Error Type

[dbrgn]

Hi

It would be nice if this library would use a custom error type for all results, instead of mixing io::Error and Strings. This would play well together with the From<Error> trait.

http://blog.burntsushi.net/rust-error-handling/

[matt2xu]

On this issue, I would propose to use the std::io::Error directly, because most sources of errors come from reading/writing to the stream. The "unexpected response code" error could be described by an ErrorKind::InvalidData.

@mattnenterprise if you're interested and would accept a pull request, this is something that I'd like to implement. This is a very useful crate in my opinion, and I have a few proposals that I feel could improve it further, and that I would be happy to implement and contribute:

• use a BufReader for parsing response codes. This simplifies read_response (thanks to the BufReader.read_line method), and should be more efficient than reading directly from the TcpStream.
• support multiple line replies (a reply that starts with "ccc-" can contain multiple lines until the last line that begins with "ccc ", ccc being the same code as the one encountered on the first line).
• similarly, use a BufWriter to write to the command stream.
• I noticed you opened an issue to allow the FTPStream to be created with anything that implements Read and Write. There could be two constructors: one with a TcpStream that creates the FtpStream with reader and writer initialized to a BufReader/BufWriter from the stream, and another one that creates the FtpStream with a given reader and a given writer.
• I suppose that FTPStream should be renamed to FtpStream from what I understand of Rust's conventions.
• Perhaps use the same function names as Node's ftp module? "get", "put", "cdup", etc.
• Have the "stor" function accept a closure with a writer as a parameter, to be able to upload data directly, rather than copying from a Read instance.

Let me know what you think about these ideas 😄

[dbrgn]

I don't have anything to say, but I like @matt2xu's ideas :)

[mattnenterprise]

@matt2xu I like the ideas you have come up with. I think they would greatly improve this crate. I would gladly accept pull requests for these changes. I would one day like this crate to be the official FTP crate for rust, and I think these changes would make it closer to that goal.

[matt2xu]

@mattnenterprise great! I'll get started this week. This will actually be my first time contributing to another project on github, I look forward to it :-)

[little-dude]

@matt2xu @dbrgn are you working on the error handling ? If not I'd give it a try.

[matt2xu]

@little-dude yes I have already implemented error handling, I was waiting for #17 to be merged first.
I think there are other things that you could work on that are potentially more interesting and quite useful too, in order of increasing complexity:

• use status codes instead of hard-coded integers in read_response calls
• rewrite current_dir to use iterators instead of inner functions
• make pasv() use the host returned by the FTP server, rather than self.host (after which probably self.host wouldn't be needed anymore, and the constructor of FtpStream could just take a ToSocketAddrs directly)
• change the API so that "stor" takes a closure for writing data. Not sure if "retr" should do the same or not?

[matt2xu]

Also, I proposed that we change some of the function names to match those in Node's ftp package, which are similar to the ftp command-line client: make_dir => mkdir, current_dir => pwd, change_dir => cwd, change_dir_to_parent => cdup, retr => get, stor => put, etc.

And of course, there's still #10 opened: implement rename, delete, append, system, etc.

[dbrgn]

Yeah, I think changing the method names to match the FTP commands makes sense. Then anybody that knows FTP will "feel at home".

I'm not working on anything else related to this library at the moment :) Maybe it's best if we create tracking issues for all of those points, if @mattnenterprise agrees that they should be implemented.

[mattnenterprise]

I agree that we should make the methods names match the ftp commands.

@matt2xu Could you make issues for the changes that you are proposing. So we can discuss those changes there, and it will be more organized.

[matt2xu]

@mattnenterprise 👍 I have created separate issues for these changes. This particular issue (error handling) is implemented in pull request #21. It's a big commit, but I feel it's worth it: the code is shorter and easier to read.

[matt2xu]

@mattnenterprise thanks for merging #21, I think we can close this issue as resolved!