[Python-bugs-list] [ python-Bugs-545096 ] ConfigParser code cleanup
noreply@sourceforge.net
noreply@sourceforge.net
Tue, 02 Jul 2002 21:08:12 -0700
Bugs item #545096, was opened at 2002-04-17 06:24
You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=105470&aid=545096&group_id=5470
Category: Python Library
Group: None
>Status: Pending
Resolution: None
Priority: 5
Submitted By: Gustavo Niemeyer (niemeyer)
Assigned to: Fred L. Drake, Jr. (fdrake)
Summary: ConfigParser code cleanup
Initial Comment:
The first patch fixes a bug, implements some speed
improvements, some memory consumption improvements,
enforces the usage of the already available global
variables, and extends the allowed chars in option
names to be very permissive.
The second one, if used, is supposed to be applied
over the first one, and implements a walk()
generator method for walking trough the options of a
section.
----------------------------------------------------------------------
>Comment By: Fred L. Drake, Jr. (fdrake)
Date: 2002-07-03 00:08
Message:
Logged In: YES
user_id=3066
Any objections to my renaming "walk" to "items" (or even
"section_items")? "walk" makes me think of os.path.walk(),
which uses a callback; what the function returns is a list
of name-value pairs, though, which makes me think "items".
Other than this, I'm ready to commit.
----------------------------------------------------------------------
Comment By: Gustavo Niemeyer (niemeyer)
Date: 2002-06-22 00:09
Message:
Logged In: YES
user_id=7887
Deal!! ;-)
Here is the updated patch.
Thanks!
----------------------------------------------------------------------
Comment By: Fred L. Drake, Jr. (fdrake)
Date: 2002-06-21 22:16
Message:
Logged In: YES
user_id=3066
Thanks! I can take care of the referred bug; I already know
how I want to solve that one. Thanks!
----------------------------------------------------------------------
Comment By: Gustavo Niemeyer (niemeyer)
Date: 2002-06-21 22:14
Message:
Logged In: YES
user_id=7887
Thanks for your review Fred!
I'll fix the patch as suggested, and work on a solution for the referred bug
as well.
----------------------------------------------------------------------
Comment By: Fred L. Drake, Jr. (fdrake)
Date: 2002-06-21 21:38
Message:
Logged In: YES
user_id=3066
Changed to a "bug" report even though it's a patch, so that
all the ConfigParser reports can show up together.
----------------------------------------------------------------------
Comment By: Fred L. Drake, Jr. (fdrake)
Date: 2002-06-21 13:22
Message:
Logged In: YES
user_id=3066
I'd like to accept a variation on this patch.
Specifically, I'd like "section in self.sections()" tests to
become "sections in self.__sections" consistently; your
proposed patch is better than the original, but as long as
we're not supporting subclassing in this way, let's go ahead
and use the better-performing solution.
I'll accept the use of MAX_INTERPOLATION_DEPTH and your
changes to the interpolation loop, but the loop itself will
probably be re-written later, in response to SF bug #511737.
The walk() method will need documentation.
Can you prepare a new patch for this?
Thanks!
----------------------------------------------------------------------
Comment By: Fred L. Drake, Jr. (fdrake)
Date: 2002-06-21 12:20
Message:
Logged In: YES
user_id=3066
Ken appears non-responsive, re-assigning to me.
----------------------------------------------------------------------
Comment By: Fred L. Drake, Jr. (fdrake)
Date: 2002-06-12 21:42
Message:
Logged In: YES
user_id=3066
Ken, can you recall our thinking on the use of the constant
for MAX_INTERPOLATION_DEPTH when this was first written? I
think you were more involved in this than the rest of us at
the time.
----------------------------------------------------------------------
Comment By: Gustavo Niemeyer (niemeyer)
Date: 2002-06-03 17:45
Message:
Logged In: YES
user_id=7887
Hello Fred!
- About has_section(), I thought about the possibility of subclassing it. But,
besides agreeing with you on the messy code matter, other functions
weren't (and still aren't) considering this possibility, so I doubt some one will
be able to subclass it anyways. Anyway, I may change the patch to use
whatever you suggest.
- About MAX_INTERPOLATION_DEPTH, I understand your position of
wanting "a safety net", but my intention of using it in the code was not to
let the user change it, but to clarify and give consistence to the code. I think
using literal constants in the code is not a good measure at all. If this was
adopted, many modules would have to be changed (in a bad way) for
consistence. With a quick look at the library, I was able to find smtpd.py,
base64.py, binhex.py, cgi.py, cmd.py, and many others using similar purpose
constants. You'll probably agree with me if you think about this more
extensively.
----------------------------------------------------------------------
Comment By: Fred L. Drake, Jr. (fdrake)
Date: 2002-05-30 12:11
Message:
Logged In: YES
user_id=3066
I should clarify: By "retain the use of the constant" I
mean the constant numeric literal, not MAX_INTERPOLATION_DEPTH.
----------------------------------------------------------------------
Comment By: Fred L. Drake, Jr. (fdrake)
Date: 2002-05-30 12:06
Message:
Logged In: YES
user_id=3066
Two comments:
- "section in self.__sections.keys()" can be written
(still more efficiently) as "section in self.__sections",
*but* the use of the sections() method is done to be
subclass-friendly. I'm not sure how important this is
since I can't imagine anyone actually subclassing
from such a mass of messy code. Perhaps these
could be modified to use the has_section() method,
which is relatively new.
- MAX_INTERPOLATION_DEPTH was not "honored"
in the code since it's really a safety net; there's no
intention that client code be able to change it. (The
sanity of the value is not checked before use.) I'm
inclined to retain the use of the constant in the
interpolation code, and add a comment above the
constant the it exists for informational purposes only.
Otherwise, I'm fine with the patch. ;-)
Sorry for the delay in responding to this.
----------------------------------------------------------------------
Comment By: Guido van Rossum (gvanrossum)
Date: 2002-04-22 13:55
Message:
Logged In: YES
user_id=6380
I'm assigning this to Fred Drake, who has some opinions on
ConfigParser.
----------------------------------------------------------------------
Comment By: Gustavo Niemeyer (niemeyer)
Date: 2002-04-18 13:07
Message:
Logged In: YES
user_id=7887
I'd rather explain here the patch that changes behavior,
since it's pretty small. This line in the regular
expression OPTCRE:
r'(?P<option>[]\-[\w_.*,(){}]+)'
was replaced by:
r'(?P<option>[^:=\s]+)'
So that instead of giving a range of characters which may
be part of the option name, it just looks for the
separator chars and spaces. This behavior is already used
in the headers, and I haven't found any good reason to
deny usage of other characters as option names.
In the same regular expression, I've also replaced
'[ \t]' by '\s', but this shouln't change the current
behavior at all.
About the walk patch, I have no idea why it isn't
attached. I remember to have checked the ticked, and it
was there. Anyway, I'm attaching it again.
----------------------------------------------------------------------
Comment By: Martin v. Löwis (loewis)
Date: 2002-04-18 02:04
Message:
Logged In: YES
user_id=21627
I'd like to see this split into even more parts: a patch
that supposedly has *no* semantic change (ie. the speed
improvements, memory consumption improvements, use of global
variables); a patch that changes behavior (please explain in
which ways); and the patch that implements walk (which
appears to be missing currently).
----------------------------------------------------------------------
You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=105470&aid=545096&group_id=5470