Author eric.araujo
Recipients eric.araujo, georg.brandl, lyren, schmir, tarek
Date 2010-09-07.13:31:52
SpamBayes Score 5.77316e-15
Marked as misclassified No
Message-id <1283866316.12.0.428230586054.issue919238@psf.upfronthosting.co.za>
In-reply-to
Content
Thank you for the patch. Review:

1) Why not raise an exception? A recursive variable definition is an error. Tarek, is this behavior change OK?

2) Does this apply to the new top-level sysconfig module and/or distutils.sysconfig?

3) I’d use a more specific name for the test method: test_bogus_variable instead of test_parse_makefile. I would also add a test for SELF=$(SELF) (currently there is only a test for cyclic definition, not recursive).

4) os.unlink(makefile) is not guaranteed to run, for example if the test is stopped or crashes. Put self.addCleanup(os.unlink, makefile) after the mkstemp call instead.

5) I didn’t know there was no “if __name__ == '__main__'” block in this test file, nice catch. There is no need for the intermediate test_main function, though: unittest.main() should be enough.

6) Minor style issues: You can use textwrap.dedent to keep the file contents in the os.write call indented; s/you're/your/; no need to del fd; put spaces around operators (here == and =); don’t add tree blank lines. See PEP 8 for more info.
History
Date User Action Args
2010-09-07 13:31:56eric.araujosetrecipients: + eric.araujo, georg.brandl, lyren, schmir, tarek
2010-09-07 13:31:56eric.araujosetmessageid: <1283866316.12.0.428230586054.issue919238@psf.upfronthosting.co.za>
2010-09-07 13:31:53eric.araujolinkissue919238 messages
2010-09-07 13:31:52eric.araujocreate