Éric Araujo <mer...@netwok.org> added the comment: 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. ---------- assignee: -> tarek nosy: +eric.araujo _______________________________________ Python tracker <rep...@bugs.python.org> <http://bugs.python.org/issue919238> _______________________________________ _______________________________________________ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
