From: | "Nathan Boley" <npboley(at)gmail(dot)com> |
---|---|
To: | "Josh Berkus" <josh(at)agliodbs(dot)com> |
Cc: | pgsql-hackers(at)postgresql(dot)org, "Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov>, "Gregory Stark" <stark(at)enterprisedb(dot)com>, jd(at)commandprompt(dot)com, "Robert Haas" <robertmhaas(at)gmail(dot)com>, "Greg Smith" <gsmith(at)gregsmith(dot)com> |
Subject: | Re: Simple postgresql.conf wizard |
Date: | 2008-12-05 20:00:38 |
Message-ID: | 6fa3b6e20812051200h274f3329k4435c7d33a660057@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Thanks for putting out pgtune - it's a sorely needed tool.
I had a chance to look over the source code and have a few comments,
mostly about python specific coding conventions.
- The windows specific try block ( line 16 ) raises a ValueError vs
ImportError on my debian machine. Maybe it would be more appropriate
to explicitly test platform.system()=="Windows"?
- from ctypes import * ( 18 ) makes the block difficult to read and
pollutes the namespace.
- on line 45, the try block should probably catch exceptions derived
from Exception ( to avoid catching SystemExit and KeyboardInterrupt
errors ). ie, except Exception: return None. Also, printing out the
expection in debug mode would probably be a good idea ( ie except
Exception, e: print e\ return None )
- all classes ( 58, 135, 205 ) are 'old-style' classes. I dont see
any reason to use classic classes ( unless Python 2.1 is a support
goal? ) To make classes 'new style' classes (
http://www.python.org/doc/2.5.2/ref/node33.html ) they should inherit
object. i.e. class PGConfigLine(object):
- The doc strings ( 59, 136, 206 ) don't follow standard conventions,
described here http://www.python.org/dev/peps/pep-0257/.
- Functions also support doc strings ( 342, 351, etc. )
- Tuple unpacking doesn't require the tuple boundaries ( 446 and
others ). ie, options, args = ReadOptions() works.
This is more of a style comment about the 'Java-ish interface' ( line
112 ), feel free to ignore it.
overloading __str__ and __repr__ are the python ways to return string
representations of an object. ie, instead of toSting use __str__ and
then ( on 197 ) print l or print str(l) instead of print l.toString()
for the other methods ( getValue, getLineNumber, isSetting ) I'm
assuming you didnt call the attributes directly because you didnt want
them to be accidently overwritten. Have you considered the use of
properties ( http://www.python.org/download/releases/2.2.3/descrintro/#property
)? Also, it would be more clear to mark attributes as private ( i.e.
_name or __name, _readable, _lineNumber, _setsParameter ) if you dont
want them to be accessed directly.
Hope my comments are useful! Thanks again for writing this.
-Nathan
P.S. I'd be happy to officially review this if it gets to that.
From | Date | Subject | |
---|---|---|---|
Next Message | Paul Ramsey | 2008-12-05 20:15:06 | Re: CLUSTER in 8.3.5 on GIST indexes loses all rows |
Previous Message | Robert Haas | 2008-12-05 19:32:57 | Re: default statistics target testing (was: Simple postgresql.conf wizard) |