{"action":"created","issue":{"url":"https://api.github.com/repos/mattnenterprise/rust-ftp/issues/47","repository_url":"https://api.github.com/repos/mattnenterprise/rust-ftp","labels_url":"https://api.github.com/repos/mattnenterprise/rust-ftp/issues/47/labels{/name}","comments_url":"https://api.github.com/repos/mattnenterprise/rust-ftp/issues/47/comments","events_url":"https://api.github.com/repos/mattnenterprise/rust-ftp/issues/47/events","html_url":"https://github.com/mattnenterprise/rust-ftp/pull/47","id":167747186,"number":47,"title":"Unify the library's interface and tidy up some code","user":{"login":"zsck","id":682761,"avatar_url":"https://avatars.githubusercontent.com/u/682761?v=3","gravatar_id":"","url":"https://api.github.com/users/zsck","html_url":"https://github.com/zsck","followers_url":"https://api.github.com/users/zsck/followers","following_url":"https://api.github.com/users/zsck/following{/other_user}","gists_url":"https://api.github.com/users/zsck/gists{/gist_id}","starred_url":"https://api.github.com/users/zsck/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/zsck/subscriptions","organizations_url":"https://api.github.com/users/zsck/orgs","repos_url":"https://api.github.com/users/zsck/repos","events_url":"https://api.github.com/users/zsck/events{/privacy}","received_events_url":"https://api.github.com/users/zsck/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":3,"created_at":"2016-07-27T01:12:43Z","updated_at":"2016-07-27T15:39:10Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/mattnenterprise/rust-ftp/pulls/47","html_url":"https://github.com/mattnenterprise/rust-ftp/pull/47","diff_url":"https://github.com/mattnenterprise/rust-ftp/pull/47.diff","patch_url":"https://github.com/mattnenterprise/rust-ftp/pull/47.patch"},"body":"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.\r\n\r\nAnother 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.\r\n\r\nYou'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.\r\n\r\nIf 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."},"comment":{"url":"https://api.github.com/repos/mattnenterprise/rust-ftp/issues/comments/235626052","html_url":"https://github.com/mattnenterprise/rust-ftp/pull/47#issuecomment-235626052","issue_url":"https://api.github.com/repos/mattnenterprise/rust-ftp/issues/47","id":235626052,"user":{"login":"mattnenterprise","id":4503095,"avatar_url":"https://avatars.githubusercontent.com/u/4503095?v=3","gravatar_id":"","url":"https://api.github.com/users/mattnenterprise","html_url":"https://github.com/mattnenterprise","followers_url":"https://api.github.com/users/mattnenterprise/followers","following_url":"https://api.github.com/users/mattnenterprise/following{/other_user}","gists_url":"https://api.github.com/users/mattnenterprise/gists{/gist_id}","starred_url":"https://api.github.com/users/mattnenterprise/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mattnenterprise/subscriptions","organizations_url":"https://api.github.com/users/mattnenterprise/orgs","repos_url":"https://api.github.com/users/mattnenterprise/repos","events_url":"https://api.github.com/users/mattnenterprise/events{/privacy}","received_events_url":"https://api.github.com/users/mattnenterprise/received_events","type":"User","site_admin":false},"created_at":"2016-07-27T15:39:10Z","updated_at":"2016-07-27T15:39:10Z","body":"1. You can put make the change, and put it into this branch.\r\n2. I didn't think of that. It should be fine that way it is for now.\r\n\r\nFor 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.\r\n\r\nOn 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 ?"}}
{"actor":{"display_login":"mattnenterprise"}}