[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