FTP.ftp_retr should check openForReading for IsADirectoryError

[Adi Roiban]

Right now FTP.ftp_retr is checking openForReading call for IsNotADirectory, while it should check for IsADirectory as shell.openForReading is raising IsADirectory exception.

I will attach a branch with the fix.

[Adi Roiban]

code is here: ​https://code.launchpad.net/~adiroiban/twisted/5441 MP is here: ​https://code.launchpad.net/~adiroiban/twisted/5441/+merge/86916

Please let me know if you want to merge/land request in another format.

Cheers

[Thijs Triemstra]

Thanks for your contribution. Our development process prefers that you supply a patch against trunk that we can review, see TwistedDevelopment#SubmittingaPatch for more information. You can also read the ​Coding standard and other ​policies for more information.

[Glyph]

thijs,

Please don't reject legitimate contributions unless you've carefully inspected them.

A launchpad merge proposal is trivial to convert to a plain text diff, so we can accept them. (Some reviewers might actually prefer them.)

If you're not familiar with bzr and launchpad, there's a button, "diff against target". which will take you here: ​https://code.launchpad.net/~adiroiban/twisted/5441/+merge/86916#review-diff

and you can click "download diff" to get a plain-text diff that should apply to trunk.

Sorry for the delay, adiroiban! I'm putting this back into review, so hopefully someone will get to it soon...

[Adi Roiban]

Hi,

I have attached a diff generated using svn.

The Launchpad Merge Proposal page is done against twisted/trunk and contains a diff web view and a link for downloading a patch file. My previous changes were accepted based on bzr branches.

Cheers, Adi

[Thijs Triemstra]

Replying to glyph:

thijs,

Please don't reject legitimate contributions unless you've carefully inspected them.

A launchpad merge proposal is trivial to convert to a plain text diff, so we can accept them. (Some reviewers might actually prefer them.)

If you're not familiar with bzr and launchpad, there's a button, "diff against target". which will take you here: ​https://code.launchpad.net/~adiroiban/twisted/5441/+merge/86916#review-diff

and you can click "download diff" to get a plain-text diff that should apply to trunk.

Alright, I wasn't aware of that. I guess the TwistedDevelopment page should point this out then, because I couldn't find anything official about the fact it's ok to use github/bitbucket/bzr/etc and let the core developers figure out how to generate a plain text diff (no matter how easy it is). The development page also suggest: "If you don't have commit access to Twisted, please submit changesets in diff -u format to the ticket tracker. "

[Glyph]

Replying to thijs:

Alright, I wasn't aware of that. I guess the TwistedDevelopment page should point this out then, because I couldn't find anything official about the fact it's ok to use github/bitbucket/bzr/etc and let the core developers figure out how to generate a plain text diff (no matter how easy it is). The development page also suggest: "If you don't have commit access to Twisted, please submit changesets in diff -u format to the ticket tracker. "

This is true, and more or less correct; ideally, contributors would also provide a patch. It's just that saying that they should generate a patch doesn't really constitute a full review :). By all means, feel free to stick a comment on a ticket saying that a patch may accelerate the process of getting it reviewed; it should just remain in the review state in case another twisted core dev is comfortable with the contributor's selected method of contribution.

-glyph

[jesstess]

Thanks for the bug report and patch, adiroiban! This patch looks good; I just have a couple of cosmetic/documentation comments:

• Tests should start with "test_", followed by a camel-case name where the first word starts lower-cased, test_downloadFolder in this case. (test_ftp.py is quite old and has a bunch of code that hasn't been updated to meet the current testing standard)
• test_downloadFolder needs a docstring.
• "Downloading a file should not succeed" ==> "Downloading a folder should not succeed"
• While you're touching ftp_RETR, can you give it a docstring?

Thanks again!

[Adi Roiban]

Thanks for the review!

I have attached a new patch and also updated the bzr branch. The patch is built on top of the previous patch. Please let me know if a full patch against the base or latest version is required.

Cheers, Adi

[Jean-Paul Calderone]

All things equal, I think most people would find a patch against trunk more useful than a patch against a bzr branch. Using the latter involves applying multiple patches or getting the bzr branch, and once you have the bzr branch you don't need the patch file anymore.

[Adi Roiban]

Hi,

I have attached a new diff against the latest trunk.

Cheers, Adi

[jesstess]

(In [33433]) Branching to 'ftpretr-directory-5441'

[jesstess]

(In [33434]) Apply patch 5441.3.diff by adiroiban.

With some tiny cosmetic tweaks (docstring """ on lines by themselves).

refs #5441

[jesstess]

Thanks for the updated patch, adiroiban. ​Buildbot results look good, so I'll go ahead and merge.

[jesstess]

(In [33435]) Merge ftpretr-directory-5441

Author: adiroiban Reviewer: jesstess Fixes: #5441

Correct a check in FTP.ftp_retr from IsNotADirectoryError to IsADirectoryError. Now calling RETR on a directory won't be logged as an unexpected error.