Author eric.araujo
Recipients benjamin.peterson, eric.araujo, georg.brandl, kontinuity, ned.deily, ronaldoussoren, srid, stoneyb, tarek
Date 2011-05-03.16:06:41
SpamBayes Score 2.78432e-07
Marked as misclassified No
Message-id <1304438802.3.0.0713101747083.issue9516@psf.upfronthosting.co.za>
In-reply-to
Content
Looks acceptable to me.  A few details in the code could be improved:

+    @unittest.skipUnless(sys.platform == 'darwin', 'MacOSX test')
Skip messages generally use another form, like “test relevant only on Mac OS X”.

+        finally:
+            os.environ = orig_environ
I’ve grown fond of using self.addCleanup(setattr, os, 'environ', os.environ.copy()) instead of try/finally.  The cleanup action can be written right before the monkey-patching line, there’s no need to indent (especially nice when you patch many things, like later in the patch with sys.stdout), and it’s less lines.

+    def _try_compile_deployment_target(self):
+        import textwrap
I’d prefer avoiding function-level imports.

+        fp.close()
I suggest a with statement.

+        tgt = '%02d%01d0'%(tgt)
I think that using real words (“target”) and following PEP 8 (“ % target”) would make this slightly more readable.

+        except CompileError:
+            self.fail("Wrong deployment target during compilation")
Why not just let the CompileError propagate and cause a unittest failure?

+        self.assertEquals(get_platform(), 'macosx-10.4-fat')
assertEquals raises a DeprecationWarning; assertEqual should be used. 
 
+
+
+
+        # Test without MACOSX_DEPLOYMENT_TARGET in the environment
+
Three blank lines, a comment line and another blank line is a lot of whitespace.

+            stderr=open('/dev/null'),
Won’t this cause a ResourceWarning?
History
Date User Action Args
2011-05-03 16:06:42eric.araujosetrecipients: + eric.araujo, georg.brandl, ronaldoussoren, benjamin.peterson, tarek, ned.deily, srid, stoneyb, kontinuity
2011-05-03 16:06:42eric.araujosetmessageid: <1304438802.3.0.0713101747083.issue9516@psf.upfronthosting.co.za>
2011-05-03 16:06:41eric.araujolinkissue9516 messages
2011-05-03 16:06:41eric.araujocreate