Opened 7 years ago

Closed 7 years ago

Last modified 6 years ago

#4207 defect closed fixed (fixed)

Make FilePath.open compatible with Python 2.7 on Windows

Reported by: Jean-Paul Calderone Owned by: Jamu Kakar
Priority: normal Milestone: Python-2.7
Component: core Keywords: easy
Cc: ivank, jesstess Branch: branches/correct-file-mode-4207
branch-diff, diff-cov, branch-cov, buildbot
Author: jkakar

Description

FilePath.open enforces that the mode is binary, so it's redundant to pass in a mode that includes "b". This worked anyway, though, as most systems treat "rb" and "rbb" the same. However, ivan pointed out that Python 2.7 on Windows appears to treat "rbb" as an error. As there's no reason to pass a "b" in in the first place, we should stop doing it in Twisted wherever we are now.

This seems mainly to be in twisted/protocols/ftp.py and twisted/plugin.py

Change History (14)

comment:1 Changed 7 years ago by Glyph

Keywords: easy added
Owner: Glyph deleted

comment:2 Changed 7 years ago by Jamu Kakar

Owner: set to Jamu Kakar
Status: newassigned

comment:3 Changed 7 years ago by Jamu Kakar

Author: jkakar
Branch: branches/correct-file-mode-4207

(In [28738]) Branching to 'correct-file-mode-4207'

comment:4 Changed 7 years ago by Jamu Kakar

Keywords: review added
Owner: Jamu Kakar deleted
Status: assignednew

The branch in review introduces the following changes:

  • Calls to FilePath.open that explicitly pass a mode including 'b' have been updated to not pass 'b', as FilePath.open automatically adds 'b'.
  • FilePath.open ensures that only a single 'b' is present in the mode that gets passed to the builtin open call, as a workaround for the bug in Python 2.7 mentioned in the description.

comment:5 Changed 7 years ago by ivank

Cc: ivank added
Keywords: review removed
Owner: set to Jamu Kakar
Summary: Assorted code in Twisted passes a mode including "b" to FilePath.openMake FilePath.open compatible with Python 2.7 on Windows

Thanks for working on this. I agree with the changes. I am changing the ticket summary to match the new primary change.

Please ignore #1 and #4 if #twisted says they're not needed due to the pending switch from pydoctor to sphinx.

  1. The new openForReading and openForWriting docstrings should have a @type for each @param
  1. In FilePath.open, please update the surrounding line mode+'b' to mode + 'b' to match the code style.
  1. In the FilePath.open docstring, I would prefer something specific for @return; for example: @return: An open C{file} object.
  1. In the FilePath.open docstring, please add a @type for the @param.
  1. In the FilePath.open docstring, please surround the 'b', 'r', 'a', and the two uses of True with a C{}.
  1. There's a typo in testOpenWithExplicitBinaryMode: buildin open should be built-in open.
  1. Same docstring: builtin should be built-in
  1. testOpenWithExplicitBinaryMode should be test_openWithExplicitBinaryMode to match the new code style.

comment:6 Changed 7 years ago by Jamu Kakar

Keywords: review added
Owner: Jamu Kakar deleted

Thanks for the review! I've addressed all your comments, please re-review.

comment:7 Changed 7 years ago by Jamu Kakar

Owner: set to ivank

comment:8 Changed 7 years ago by Jamu Kakar

Owner: ivank deleted

comment:9 Changed 7 years ago by jesstess

Owner: set to jesstess

comment:10 Changed 7 years ago by jesstess

Cc: jesstess added
Keywords: review removed
Owner: changed from jesstess to Jamu Kakar

Thanks for working on this, jkakar! Some feedback, mostly for minor cleanup in surrounding parts of the files that have been changed:

test_paths.py:

  • needs a copyright bump
  • has the unused variable basedir
  • class FilePathTestCase needs a docstring
  • test_openWithExplicitBinaryMode is testing 2 different cases, which should be split into separate test cases.
  • In the 'bb' test case, without the changes in filepath.py would we be seeing a ValueError? If so, the test should probably show that that doesn't get raised, in addition to the fact that you can still stat the file with the writer.exists check.

filepath.py:

  • needs a copyright bump
  • FilePath.open's docstring needs an @rtype
  • the assert in open goes past 80 columns
  • FilePath is missing epytext markup for it's path ivar

Additionally:

  • ftp.py defines e for errors several times but never uses it
  • plugin.py needs a copyright bump

comment:11 Changed 7 years ago by Jamu Kakar

Keywords: review added
Owner: changed from Jamu Kakar to jesstess

jesstess thanks for the review! I've addressed all your comments, except one:

I don't think the test for the multiple 'bb' case needs to assert that ValueError isn't raised, because adding such an assertion wouldn't make the test easier to understand. If it does get raised for some reason it can bubble up and cause a failure when the test is run.

Also, in plugin.py there is a copyright header for Divmod and another for Twisted. I've only updated the Twisted one, but should I also update the Divmod one?

comment:12 Changed 7 years ago by jesstess

Keywords: review removed
Owner: changed from jesstess to Jamu Kakar

Thanks for working on this, jkakar. As an aside, a #refs <ticket number> in a commit message will add a track comment on the given ticket with the commit message, if that's a facility you'd find useful. (More on this in the Twisted svn reference)

In plugin.py, the Divmod copyright should stay where it is, but the Twisted copyright still needs a bump.

Other than that, the builds look good and I think this is ready to merge!

comment:13 Changed 7 years ago by Jamu Kakar

Resolution: fixed
Status: newclosed

(In [28793]) Merge correct-file-mode-4207

Author: jkakar Reviewers: ivank, jesstess Fixes: #4207

Due to a bug in Python 2.7a on Windows, including multiple 'b' characters in the mode passed to the built-in open() will cause an error. FilePath.open has been modified to ensure that only a single 'b' character is included in the mode passed to the built-in open() to avoid this issue. Callsites that pass a redundant 'b' mode have been updated to FilePath.open no longer do so, since it ensures that 'b' is always present in the mode. This branch also includes some drive-by formatting cleanups and copyright updates.

See http://bugs.python.org/issue7686 for details about the bug in Python 2.7a that necessitated this change.

comment:14 Changed 7 years ago by ivank

Milestone: Python-2.7
Note: See TracTickets for help on using tickets.