[Python-Dev] PEP 282 Implementation

Guido van Rossum guido@python.org
Mon, 23 Sep 2002 13:41:14 -0400


I'm sorry that this seems to be a thread with one message per month!
I'll try to be more responsive from now on, the big Zope projects that
were keeping me busy have given me some slack time.

> > In general the code looks good.  Only one style nits: I prefer
> > docstrings that have a one-line summary, then a blank line, and then a
> > longer description.
> 
> I will update the docstrings as per your feedback.

Great!  (When can we see a new release on
http://www.red-dove.com/python_logging.html
?)

> > There's a lot of code there!  Should it perhaps be broken up into
> > different modules?  Perhaps it should become a logging *package* with
> > submodules that define the various filters and handlers.
> 
> How strongly do you feel about this? I did think about doing this
> and in fact the first implementation of the module was as a
> package. I found this a little more cumbersome than the single-file
> solution, and reimplemented as logging.py. The module is a little on
> the large side but the single-file organization makes it a little
> easier to use.

I would feel much less strongly about this if several of the
additional things could be moved to separate files without making it a
package.

> > - Why does the FileHandler open the file with mode "a+" (and later
> >   with "w+")?  The "+" makes the file readable, but I see no
> >   reason to read it.  Am I missing?
> 
> No, you're right - using "a" and "w" should work. I'll change the
> code to lose the "+".

OK.

> > - setRollover(): the explanation isn't 100% clear.  I *think* that
> >   you always write to "app.log", and when that's full, you rename
> >   it to app.log.1, and app.log.1 gets renamed to app.log.2, and so
> >   on, and then you start writing to a new app.log, right?
> 
> Yes. The original implementation was different - it just closed the
> current file and opened a new file app.log.n. The current
> implementation is slightly slower due to the need to rename several
> files, but the user can tell more easily which the latest log file
> is. I will update the setRollover() docstring to indicate more
> clearly how it works; I'm assuming that the current algorithm is
> deemed good enough.

Yes, this seems how log rotation is generally done.  (Please remove
the commented-out old code.)

> > - class SocketHandler: why set yourself up for buffer overflow by
> >   using only 2 bytes for the packet size?  You can use the struct
> >   module to encode/decode this, BTW.  I also wonder what the
> >   application for this is, BTW.
> 
> I agree about the 2-byte limit. I can change it to use struct and an
> integer length. The application for encoding the length is simply to
> allow a socket-based server to handle multiple events sent by
> SocketHandler, in the event that the connection is kept open as long
> as possible and not shut down after every event.

OK, please change it to a 4-byte length header.

I understand why you need the length header; I'm just curious about
the need for a socket server.

> >   - method send(): in Python 2.2 and later, you can use the
> >     sendall() socket method which takes care of this loop for you.
> 
> OK. I can update the code to use this in the case of 2.2 and later.

Especially since this is slated to go into 2.3 only. :-)

> > - class DatagramHandler, method send(): I don't think UDP handles
> >   fragmented packets very well -- if you have to break the packet up,
> >   there's no guarantee that the receiver will see the parts in order
> >   (or even all of them).
> 
> You're absolutely right - I wasn't thinking clearly enough about how
> UDP actually works. I will replace the loop with a single sendto()
> call.

The length header might still be useful just to be format-compatible
with the TCP variant though.

> > - fileConfig(): Is there documentation for the configuration file?
> 
> There is some documentation in the python_logging.html file which is
> part of the distribution and also on the Web at
> http://www.red-dove.com/python_logging.html - it's in the form of
> comments in an annotated logconf.ini. I have not polished the
> documentation in this area as I'm not sure how much of the
> configuration stuff should be in the logging module itself. Feedback
> I've had indicates that at least some people object moderately
> strongly to having a particular configuration design forced on
> them. I'd appreciate views on this.

This is an example of something that I'd like to see relegated to a
separate file.  It really looks like fileConfig(), listen() and
stopListening() are a separate feature bundle that looks like it is
a specific example application rather than a core feature of the
logging module.  It certainly doesn't appear in PEP 282.  Maybe the
socket handler classes belong in the same category.

Of course, the same can be said about all Handler subclasses except
StreamHandler.  Only StreamHandler is referenced by basicConfig().
Perhaps these should all (except StreamHandler) be moved to separate
files?  This sounds like a reason to make it a package.  The main
logging code could be in the __init__.py file -- there's no rule that
says __init__.py should be empty or short!

PS. In your comments you seem fond of the word "needful".  I've rarely
heard that word -- perhaps it is archaic or common only in India?

--Guido van Rossum (home page: http://www.python.org/~guido/)