classification
Title: ConfigParser behavior change
Type: Stage:
Components: Library (Lib) Versions: Python 2.4
process
Status: closed Resolution: accepted
Dependencies: Superseder:
Assigned To: fdrake Nosy List: anthonybaxter, fdrake, goodger, rhettinger
Priority: high Keywords:

Created on 2004-07-24 12:30 by goodger, last changed 2004-10-03 16:07 by goodger. This issue is now closed.

Files
File name Uploaded Description Edit
ConfigParser.py.997050.diff goodger, 2004-09-23 14:28 patch for Lib/ConfigParser.py
test_cfgparser.py.997050.diff goodger, 2004-09-23 14:29 patch for Lib/test/test_cfgparser.py
libcfgparser.tex.997050.diff goodger, 2004-09-23 14:30 patch for Doc/lib/libcfgparser.tex
Messages (11)
msg21779 - (view) Author: David Goodger (goodger) (Python committer) Date: 2004-07-24 12:30
ConfigParser.set() doesn't allow non-string arguments
for 'value' any
more.  This breaks Docutils code.  I submit that the
behavior should
not have been changed, rather that the documentation
needed updating.
I volunteer to undo the change and update the
documentation.

ConfigParser.py rev. 1.65 implements the fix detailed
in bug report
810843 (http://python.org/sf/810843):

    Fixed using methods (2) and (3): ConfigParser.set()
now checks
    that the value is some flavor of string, and raises
TypeError if
    it isn't.  This is documented behavior; regression
tests have been
    added.

"This is documented behavior": where was this behavior
documented
before the bug fix?  I couldn't find it.  If it wasn't
documented,
wouldn't method 4 (from the bug report, below) have
been more
appropriate?

    (4) Stating in the documentation that the raw parameter
    should be used when an option's value might not be of
    type string (this might be the intended usage, but it
    isn't made clear).

IOW, perhaps it wasn't a code bug but rather an
omission in the docs.
By adding the restriction and raising TypeError, it
breaks Docutils,
which subclasses ConfigParser.  If the string-only
restriction *was*
documented and I just missed it, I'll accept that the
Docutils code
was at fault (it's not the first time ;-) and rework
it.  But if that
restriction wasn't documented, I don't think
ConfigParser's behavior
should change.

See also
<http://article.gmane.org/gmane.text.docutils.devel/2073>.
msg21780 - (view) Author: David Goodger (goodger) (Python committer) Date: 2004-08-24 15:00
Logged In: YES 
user_id=7733

The change that I'm proposing to revert:
http://cvs.sourceforge.net/viewcvs.py/python/python/dist/src/Lib/ConfigParser.py?r1=1.64&r2=1.65
msg21781 - (view) Author: David Goodger (goodger) (Python committer) Date: 2004-08-24 15:04
Logged In: YES 
user_id=7733

The alternative is the following change in Docutils. 
Unfortunately, it requires access to internal implementation
details marked with single leading underscores.

diff -u -r1.64 frontend.py
--- docutils/frontend.py	24 Jul 2004 14:13:38 -0000	1.64
+++ docutils/frontend.py	24 Aug 2004 15:01:32 -0000
@@ -637,6 +637,22 @@
                 section_dict[option] = self.get(section,
option, raw=1)
         return section_dict
 
+    def set(self, section, option, value):
+        """
+        Set an option.
+
+        Overrides stdlib ConfigParser's set() method to
allow non-string
+        values.  Required for compatibility with Python 2.4.
+        """
+        if not section or section == CP.DEFAULTSECT:
+            sectdict = self._defaults
+        else:
+            try:
+                sectdict = self._sections[section]
+            except KeyError:
+                raise CP.NoSectionError(section)
+        sectdict[self.optionxform(option)] = value
+
 
 class ConfigDeprecationWarning(DeprecationWarning):
     """Warning for deprecated configuration file features."""
msg21782 - (view) Author: David Goodger (goodger) (Python committer) Date: 2004-08-24 15:10
Logged In: YES 
user_id=7733

Also see the python-dev thread at
<http://mail.python.org/pipermail/python-dev/2004-August/046607.html>.
The third message in the thread was garbled due to a GPG
problem; reproduced below.

[Fred L. Drake, Jr.]
> David has (properly) been asking me to look into this, and
i've
> managed not to have enough time.  Sorry, David!

I thought a python-dev nudge might get you off your keister ;-)

> The ConfigParser documentation was certainly too vague,
but the
> problem, as I see it, is that the module was never
intended to store
> non-string values.

BUT ConfigParser *did* allow non-strings to be stored, for
internal
use only (can't write them out, of course).  And changing it
*did*
break code.  I think a doc change acknowledging that ability but
qualifying it ("for internal use only, can't be used to
write out
config files or interpolate values") would have been a
better fix.

The change in rev. 1.65 makes ConfigParser.py *less* useful.
 When I
used ConfigParser in Docutils, I wasn't being especially
clever when I
used it to set non-string values.  It will take a lot more
cleverness
post-rev-1.65 to enable that functionality.

Of course, the fix to Docutils is pretty simple: just override
ConfigParser.set with the older version.  But that depends
on private
implementation details (self._defaults, self._sections). 
This is
*much* more dangerous and impossible to future-proof against.

> I think allowing them is just asking for still more
trouble from
> that module down the road.

Has there been any trouble until now?  Why "fix" something
that wasn't
really broke?

The original bug report (http://www.python.org/sf/810843)
was really
only asking for clarification.  It ends with:

    Nonetheless, I was shocked to see what I thought was a
    straightforward use of ConfigParser throw off errors.

Imagine developers' shock now that we can't do what we want
at all!

Docutils is not the only project using ConfigParser to store
non-strings.  I wouldn't be surprised to see other code
breaking,
which we probably wouldn't find out about until after 2.4-final.

> Sure, the module could be made happy by reverting the
patch you
> cite, but happy-by-accident is a very fragile state to be in.

So let's remove the accident, and make the happiness official!

>> The comment for bug 810843 says "This is documented
behavior", but
>> I couldn't find any such documentation pre-dating this
change.
>
> This may have been a bug in my memory.

Now that the memory bug is fixed, how about reconsidering the
resultant decision?

I'll write a doc patch ASAP.

-- 
David Goodger <http://python.net/~goodger>
msg21783 - (view) Author: David Goodger (goodger) (Python committer) Date: 2004-08-24 15:14
Logged In: YES 
user_id=7733

I strongly feel that the 2.3 behavior should be restored,
and documented, before 2.4b1.  I will do the doc change.

I have updated the bug report with links and discussions.
msg21784 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2004-08-24 16:10
Logged In: YES 
user_id=80475

Perhaps, the set() method should coerce to a string rather
than test for it.
msg21785 - (view) Author: David Goodger (goodger) (Python committer) Date: 2004-08-25 03:34
Logged In: YES 
user_id=7733

[rhettinger]
> Perhaps, the set() method should coerce to a string rather
> than test for it.

Perhaps.  But regardless of whether it's a test or coercion,
if it's in RawConfigParser.set, it prevents non-string use.
Perhaps ConfigParser.set should extend RawConfigParser.set
with the string test or coercion, leaving
RawConfigParser.set as it was before rev 1.65.

That may be the best solution.
msg21786 - (view) Author: Anthony Baxter (anthonybaxter) Date: 2004-08-30 16:18
Logged In: YES 
user_id=29957

I can live with David's last solution (moving the type check
to CP.set). RCP isn't in __all__, and isn't documented, so I
don't think it matters so much if it has internally
inconsistent results - we can also put a type check in the
output code to make sure it gets picked up there...

msg21787 - (view) Author: David Goodger (goodger) (Python committer) Date: 2004-08-30 17:26
Logged In: YES 
user_id=7733

[anthonybaxter]
> RCP isn't in __all__,

Perhaps it should be, because:

> and isn't documented,

Actually, it is documented:
http://www.python.org/dev/doc/devel/lib/RawConfigParser-objects.html

> we can also put a type check in the
> output code to make sure it gets picked up there...

It gets more and more complicated, doesn't it?  I still
think that the code itself wasn't broken so there was no
need to fix it.  The simplest solution would be to revert
the rev 1.65 code changes, and just add some documentation.
 Something along the lines of: "Output only works with
string values. If used for non-string values, you're on your
own."

We shouldn't be *removing* functionality without a darned
good reason.  I don't see such a reason here.  And since
this change broke preexisting code that relied on that
functionality, it seems obvious that it should be reverted.
msg21788 - (view) Author: David Goodger (goodger) (Python committer) Date: 2004-09-23 14:28
Logged In: YES 
user_id=7733

I have come up with a solution that should satisfy everyone.
 Python 2.4's ConfigParser.py has added a new
SafeConfigParser class, so the new string-only restriction
should be added to the new class. Existing behavior is now
documented and remains valid, so there should be no code
breakage.  But new users will get the "safe" string-only
behavior from SafeConfigParser.

Patches added for ConfigParser.py, test_cfgparser.py, and
libcfgparser.tex.

This should be applied prior to Python 2.4-beta-1.
msg21789 - (view) Author: David Goodger (goodger) (Python committer) Date: 2004-10-03 16:07
Logged In: YES 
user_id=7733

Applied the patches in CVS.

Lib/ConfigParser.py 1.68
Lib/test/test_cfgparser.py 1.25
Doc/lib/libcfgparser.tex 1.39
History
Date User Action Args
2004-07-24 12:30:16goodgercreate