Opened 11 years ago
Closed 10 years ago
#5441 defect closed fixed (fixed)
FTP.ftp_retr should check openForReading for IsADirectoryError
Reported by: | Adi Roiban | Owned by: | jesstess |
---|---|---|---|
Priority: | normal | Milestone: | |
Component: | ftp | Keywords: | |
Cc: | itamarst, Thijs Triemstra, jesstess | Branch: |
branches/ftpretr-directory-5441
branch-diff, diff-cov, branch-cov, buildbot |
Author: | jesstess |
Description (last modified by )
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.
Attachments (3)
Change History (21)
comment:1 Changed 11 years ago by
Cc: | itamarst added |
---|
comment:2 Changed 11 years ago by
Keywords: | review added |
---|---|
Owner: | set to Adi Roiban |
comment:3 Changed 11 years ago by
Cc: | Thijs Triemstra added |
---|---|
Description: | modified (diff) |
Keywords: | review removed |
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.
comment:4 follow-up: 6 Changed 11 years ago by
Keywords: | review added |
---|
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...
Changed 11 years ago by
comment:5 Changed 11 years ago by
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
comment:6 follow-up: 8 Changed 11 years ago by
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. "
comment:7 Changed 11 years ago by
Owner: | Adi Roiban deleted |
---|
comment:8 Changed 10 years ago by
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
comment:9 Changed 10 years ago by
Cc: | jesstess added |
---|---|
Owner: | set to jesstess |
comment:10 Changed 10 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from jesstess to Adi Roiban |
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!
Changed 10 years ago by
Attachment: | 5441.2.diff added |
---|
comment:11 Changed 10 years ago by
Keywords: | review added |
---|
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
comment:12 Changed 10 years ago by
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.
Changed 10 years ago by
Attachment: | 5441.3.diff added |
---|
comment:13 Changed 10 years ago by
Hi,
I have attached a new diff against the latest trunk.
Cheers, Adi
comment:14 Changed 10 years ago by
Author: | → jesstess |
---|---|
Branch: | → branches/ftpretr-directory-5441 |
(In [33433]) Branching to 'ftpretr-directory-5441'
comment:15 Changed 10 years ago by
Owner: | changed from Adi Roiban to jesstess |
---|
comment:16 Changed 10 years ago by
comment:17 Changed 10 years ago by
Keywords: | review removed |
---|
Thanks for the updated patch, adiroiban. Buildbot results look good, so I'll go ahead and merge.
comment:18 Changed 10 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
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