search

mattnenterprise / rust-ftp

FTP client for Rust
Apache License 2.0
182 stars 57 forks source link

Unify the library's interface and tidy up some code #47

Closed arcrose closed 8 years ago

arcrose commented 8 years ago

I realize that this is a fairly controversial and large pull request. I've been using rust-ftp a fair bit lately and have found the code a bit confusing to navigate at times. It is evident that you, Matt, were improving your Rust skills as you wrote the library and I can see you've become a lot more comfortable with error handling in particular as you progressed. Because of the library's earlier roots, some code was implemented with an unnecessarily large amount of Result unwrapping, by way of match. One of the big things I set out to do here is make the implementation of each method more consistent.

Another thing I wanted to set out to do here was introduce a custom error type, FtpError, so that the library only ever returns that one error type in its interface. This allows for much simpler usage, as a user could now chain operations together using Result.and_then() without having to do much intermediate error type inspection and conversion.

You'll also see that I have changed the return types of the messier secure() and secure_with_ssl() methods, which had previously returned a tuple containing a (hopefully) modified FtpStream and an io::Result<()> in the case of error.

If you'd like for me to elaborate on any of the choices I made or would like to discuss the impact or desirability of any of these changes, please don't hesitate to let me know. Since this refactoring is so substantial, I understand if there are parts you disagree with. If you feel that the changes I made are generally not in the spirit of your own intentions, you can reject this PR and I will create my own distinct version of the library.

mattnenterprise commented 8 years ago

I was actually planning on getting to adding the error stuff eventually. I have already done something similar in my imap library. I do have a couple of questions to ask.

  1. Would it be better to define our own Result type ? This would be much like how io defines it own Result. For example you could define pub type Result<T> = result::Result<T, FtpError>; Then you could return this result from each function.
  2. Could we put an SslError in the SecureError instead of a String ? It seems that SSL library returns an SslError. If we just wrap it you could programmatically get information out of the error instead of a string that is harder to match against programmatically.

Other than that I really like the changes!

arcrose commented 8 years ago

Regarding your two questions:

  1. Absolutely, we can do that. I can either do that this evening and push it into this branch or, if you'd like to merge this before then, you can certainly add it where it makes sense.
  2. I had actually tried to put SslError inside of SecureError, with a definition like the one below, however I found that I could no longer match on FtpError to get the SslError out if the "secure" flag wasn't enabled, because I'd get an error about SslError not being imported. If you already know how to deal with that scenario, by all means please do implement it. It would absolutely be an improvement.

Regarding the Travis build; do you have any suggestions about what to do to get it passing? I've verified that example code still works locally, and got the test_ftp example working locally by tweaking the credentials to match my setup. So my confidence that this works is fairly high, but I definitely would like for the build to be passing.

mattnenterprise commented 8 years ago
  1. You can make the change, and put it into this branch.
  2. I didn't think of that. It should be fine the way it is for now.

For the Travis build we setup a FTP server. I believe it is configured to use the username anonymous. That could be why it is failing. We really don't have a lot of testing at the moment, and I would like to at least have a decent amount of coverage.

On the testing side of things I was thinking of creating a fake FTP server in Rust, and testing against it. Rather than needing an FTP server installed locally for testing. What do you think about this idea ?

arcrose commented 8 years ago

I'd actually love for this library to be able to also supply functionality for a server. It would help my own use case as well, and give us a full package.

I'll make a point to also change the credentials in the test_ftp back to the anonymous username you mentioned when I make my other changes.

arcrose commented 8 years ago

Nice. Reverting to the old credentials fixed the build. Also got that custom type implemented.

mattnenterprise commented 8 years ago

I'm going to merge this in. I'm also going to update the crate version.