launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:open-for-writing-errors into launchpad:master.

Commit message:
Re-raise unexpected I/O errors from open_for_writing

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/399285

I don't think it can have been the intended behaviour for this to return None rather than raising an exception for errors other than the directory not existing.  Any such case would almost certainly fail shortly afterwards anyway due to trying to treat None as a file object, but the resulting exception would be much less clear.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:open-for-writing-errors into launchpad:master.

diff --git a/lib/lp/services/osutils.py b/lib/lp/services/osutils.py
index c670e62..38c8783 100644
--- a/lib/lp/services/osutils.py
+++ b/lib/lp/services/osutils.py
@@ -98,6 +98,7 @@ def open_for_writing(filename, mode, dirmode=0o777):
         if e.errno == errno.ENOENT:
             os.makedirs(os.path.dirname(filename), mode=dirmode)
             return open(filename, mode)
+        raise
 
 
 def _kill_may_race(pid, signal_number):
diff --git a/lib/lp/services/tests/test_osutils.py b/lib/lp/services/tests/test_osutils.py
index bbf4300..73359a6 100644
--- a/lib/lp/services/tests/test_osutils.py
+++ b/lib/lp/services/tests/test_osutils.py
@@ -100,6 +100,14 @@ class TestOpenForWriting(TestCase):
         with open(filename) as fp:
             self.assertEqual("Hello world!\n", fp.read())
 
+    def test_error(self):
+        # open_for_writing passes through errors other than the directory
+        # not existing.
+        directory = self.makeTemporaryDirectory()
+        os.chmod(directory, 0)
+        filename = os.path.join(directory, 'foo')
+        self.assertRaises(IOError, open_for_writing, filename, 'w')
+
 
 class TestWriteFile(TestCase):