On Wed, Apr 6, 2011 at 12:04 PM, Lukas Fleischer <archli...@cryptocrack.de> wrote: > I agree with both changes, but please split that one into two separate > patches.
ugh. yeah I can probably do that. >> -DBUG = 1 >> +log_level = logging.DEBUG # logging level. set to logging.INFO to reduce >> output > > I'm not a Python coder, but is there any reason to use lowercase here > whereas we use uppercase for all other constants? No reason really. I am just not used to uppercasing variables. When I refactor to split the above changes I will try to remember to make that variable format similar to the others. >> raise SystemExit > > Shouldn't we rather use "sys.exit(1);" here instead of raising a > SystemExit exception? That way we'd have a proper exit status, also. > Might be something to include in the debugging/error handling patch. Possibly. sys.exit actually raises SystemExit, if I remember correctly. Setting a shell exit value is a good idea though. I will add that in. >> num_comments = random.randrange(PKG_CMNTS[0], PKG_CMNTS[1]) >> for i in range(0, num_comments): >> - fortune = esc(commands.getoutput(FORTUNE_CMD).replace("'","")) >> + fortune = commands.getoutput(FORTUNE_CMD).replace("'","") > > Why did you drop escape_string() here? It relies upon mysql, and since the other instance of mysql usage was removed by one of my patches, I removed this as well (to remove the dep entirely). For dummy data there really isn't a danger of sql injection, and removing ' characters from the fortune_cmd result string should be enough to keep from causing the written sql to be badly formatted.