Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(736)

Issue 3309043: Roundup issue #10499: Modular interpolation in configparser

Can't Edit
Can't Publish+Mail
Start Review
Created:
13 years, 5 months ago by Lukasz.Langa (ambv)
Modified:
9 years, 7 months ago
Reviewers:
fdrake
Base URL:
http://svn.python.org/projects/python/branches/py3k/
Visibility:
Public.

Patch Set 1 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+185 lines, -203 lines) Patch
M Lib/configparser.py View 9 chunks +177 lines, -201 lines 2 comments Download
M Lib/test/test_cfgparser.py View 3 chunks +8 lines, -2 lines 0 comments Download

Messages

Total messages: 1
fdrake
13 years, 5 months ago (2010-11-28 22:56:23 UTC) #1
- Still needs tests of passing in an interpolation.

- When there's no interpolation, using an identity interpolation instead can
simplify the logic, avoiding the "if self._interpolation is None" bits.

- Needs documentation.

http://codereview.appspot.com/3309043/diff/1/Lib/configparser.py
File Lib/configparser.py (right):

http://codereview.appspot.com/3309043/diff/1/Lib/configparser.py#newcode508
Lib/configparser.py:508: if self.__class__ is RawConfigParser:
I'd like to discourage this deprecation; I've been recommending RawConfigParser
for a long time, and it still seems a reasonable way to ask for cross-version
code to request no interpretation.

http://codereview.appspot.com/3309043/diff/1/Lib/configparser.py#newcode545
Lib/configparser.py:545: self._interpolation = interpolation()
I'd rather see *interpolation* be an object rather than a factory.  We really
don't care how it's instantiated, and it might take construction parameters.  If
we don't need to be involved in the construction of the object, let's not be.

The default interpolations can probably be pre-constructed instances as well,
though I don't think that matters so much.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b