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.
