Fred L. Drake, Jr. <fdr...@acm.org> added the comment: (Apparently I don't have the right permissions on Rietveld.)
- Docstrings should be written in the standard PEP-8 way (single line summary + additional explanation as needed following a blank line). - read_sting and read_dict should still take a `filename` argument for use in messages, with <string> and something like <data in ...> (with the caller's __file__ being filled in if not provided). - Indentation in the last read_dict of test.test_cfgparser.BasicTestCase.test_basic_from_dict is incconsistent with the previous read_dict in the same test. - Lines over 79 characters should be shortened. Most of these are in docstrings, so just re-wrapping should be sufficient for most. - Changing the test structure to avoid self.cf may have been convenient, but is unrelated to the actual functionality changes. In the future such refactorings should be performed in separate patches. (Ordering dependencies are fine, so long as they're noted in the relevant issues.) - DuplicateOptionError is missing from __all__. - Changing the constructor to use keyword-only arguments carries some backward-compatibility risk. That can be avvoided by removing that change and adding strict=False at the end of the parameter list. It's unlikely that this is a significant risk, since these parameters generally lend themselves to keyword usage. I think this should have been several separate patches: - refactoring (the self.cf changes in the tests) - addition of the DuplicateOptionError - the read_* methods (including the readfp deprecation) - the new "strict" option Don't change that at this point, but please consider smaller chunks in the future. ---------- _______________________________________ Python tracker <rep...@bugs.python.org> <http://bugs.python.org/issue9452> _______________________________________ _______________________________________________ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
