This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

Author lukasz.langa
Recipients brian.curtin, ezio.melotti, fdrake, lukasz.langa
Date 2010-08-05.22:16:04
SpamBayes Score 1.2767565e-15
Marked as misclassified No
Message-id <1281046580.52.0.134218214353.issue9452@psf.upfronthosting.co.za>
In-reply-to
Content
Updated patch after review by Fred Drake. Thanks, it was terrific!

Status:

> Docstrings should be written in the standard PEP-8 way (single line
> summary + additional explanation as needed following a blank line).

Corrected where applicable. Is it OK if the one-sentence summary is occasionally longer than one line? Check out DuplicateSectionError, could it have a summary as complete as this that would fit a single line?

On a similar note, an inconsistency of configparser.py is that sometimes a blank line is placed after the docstring and sometimes there is none. How would you want this to look vi in the end?

> 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).

Are you sure about that? `read_string` might have some remote use for an argument like that, but I'd call it source= anyway. As for `read_dict`, the implementation does not even use _read(), so I don't know where could this potential `filename=` be used.

> 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.

Updated. All in all, some major unit test lipsticking should be done as a separate patch.

> Lines over 79 characters should be shortened.  Most of these are in
> docstrings, so just re-wrapping should be sufficient for most.

Corrected, even made my Vim show me these kinds of formatting problems. I also corrected a couple of these which were there before the change.

> 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.)

Good point, thanks.

> DuplicateOptionError is missing from __all__.

Corrected.

> 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.

All of these arguments are new in trunk so there is no backwards compatibility here to think about. The arguments that are not new (defaults and dict_type) are placed before the asterisk.

> Don't change that at this point, but please consider smaller chunks in
> the future.

I will, thanks.
History
Date User Action Args
2010-08-05 22:16:21lukasz.langasetrecipients: + lukasz.langa, fdrake, ezio.melotti, brian.curtin
2010-08-05 22:16:20lukasz.langasetmessageid: <1281046580.52.0.134218214353.issue9452@psf.upfronthosting.co.za>
2010-08-05 22:16:19lukasz.langalinkissue9452 messages
2010-08-05 22:16:12lukasz.langacreate