micahflee
commented
11 years ago I had no idea you could do a pull request from branches in the same repository. That's cool.
Closed dtauerbach closed 11 years ago
micahflee
commented
11 years ago I had no idea you could do a pull request from branches in the same repository. That's cool.
If the tests pass, feel free to merge this branch into master -- I noticed one test still needs to be re-implemented but that's OK. The refactoring looks great overall, but I haven't run or tested the code. Inheriting from Message is a great idea, and simplifies the code. Nice job doing away with the unnecessary 'properties' variable too, and identifying the GPG_HOMEDIR config value.
A couple of comments for looking ahead, not things that need to be changed in this code:
(1) I don't think we should be so cavalier in creating a new key for the bot. Creating a new key should be doable, for sure, but maybe with a command-line flag that has to be explicitly passed in or something like that. I think the normal behavior should be failure if a key doesn't exist. In fact, you should be able to pass in a fingerprint and the code should fail if a private key cannot be found with this fp. This is because we are going to widely publish some key and say "encrypt this to key 13374A1BEEF..." We want to be sure to fail if we can't find that key for some reason, as opposed to generating a new one.
(2) There should be a graceful failure if OpenPGPMessage doesn't have a keyring it can read, or can't decrypt a message.
(3) Much more minor stylistic comment. Mental note to move the check_bot_keypair function call into main, and break out the logic in main into more functions if desired, assuming no objections? Just prefer keeping this orderly, especially if we add argparse code, but very minor and I don't care too much.