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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
