Lukasa / requests-ftp

An FTP transport adapter for use with the Python Requests library.
Other
69 stars 25 forks source link

Fixes #3 #4

Closed asmeurer closed 10 years ago

asmeurer commented 10 years ago

I have no idea about any of this. Don't merge unless you agree with every change, because I really have no idea what I am doing.

:wink:

Lukasa commented 10 years ago

Seemed like a perfect moment for this dog:

I HAVE NO IDEA WHAT I'M DOING

Lukasa commented 10 years ago

Alright, the core problem is that I assumed we shouldn't call .login() for anonymous FTP, and because I didn't actually test with an anonymous FTP server I never found out that was wrong. Lesson learned.

The correct fix is to change the logic here to read:

if auth is not None:
    self.conn.login(auth[0], auth[1])
else:
    self.conn.login()

I've noted in the diff which things you need to pull out, and you'll want to add the above in: then everything should be wonderful.

Lukasa commented 10 years ago

BTW, thanks for this! I've mentioned before that this wasn't really intended as a serious project: it was intended as a companion project to this blog post that would demonstrate the power of Transport Adapters. With that said, I'm excited by the idea that someone might actually use it!

Lukasa commented 10 years ago

I should note also that if conda really intends to use this long term I'll need to set aside time to improve the general quality of this library, including writing some tests.

asmeurer commented 10 years ago

We won't depend on this as a library, unless it is in requests itself. I have just copied the code for this into conda (still in a pull request). But I will keep an eye on this project.

asmeurer commented 10 years ago

I don't know if ftp even supports this, but I noticed there is no content-length in the header, so there is no way for us to provide a download progress bar for ftp downloads.

asmeurer commented 10 years ago

I guess urllib doesn't support that either. So no regression.

Lukasa commented 10 years ago

Vendoring this is fine, and makes a lot of sense for package managers. I'd argue that's an implicit dependency, but really it's just semantics. =)

Otherwise, this looks great. Let's do it. :shipit: