| msg21779 - (view) |
Author: David Goodger (goodger)  |
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) |
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) |
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) |
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) |
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) * |
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) |
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) |
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) |
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) |
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 |
|
|