Code review comment for lp:~leonardr/launchpadlib/improve-workflow

Nice cover letter.

> Since it's now possible to distinguish "I don't want you to have access to my Launchpad account" from "I haven't made a decision about giving you access", I took the opportunity to improve launchpadlib's behavior when access is denied. Previously launchpadlib just raised an unhelpful HTTPError. Now it ensures that login_with() and the like return None instead of a Launchpad object. When you call login_with(), you can check the return value and know that if it's None, the user made a decision not to give you access--you can sys.exit() or complain or whatever.

This of course needs to go into the docs and I would say ideally to get a brief post on the blog (even just a pointer to this mp.)

I think you should raise a specific exception if the user denies access, rather than returning None:

1- If the app ignores the return code, we don't want it to just blithely continue and then cause knock-on errors. The default should be for the app to stop.
2- Returning None for errors is just bad.
3- It gives you a space to eventually possibly pass back a sensible message. (I mean I doubt the user wants to explain why, but perhaps you can also in the future say "this app is banned" or "you're creating too many tokens" etc.) I would say you should probably put the error body in to the exception, even if at the moment it's not super helpful.

+ print " (%s)" % self.authorization_url
+ print "should be opening in your browser. Use your browser to"
+ print "to authorize this program to access Launchpad on your behalf."

It would be nice to not directly print but rather pass this to a callback function so guis can hook it. It could be a followon but perhaps you could just hoist it out while you're here.

- launchpad.credentials.save_to_path(consumer_credentials_path)
- os.chmod(consumer_credentials_path, stat.S_IREAD | stat.S_IWRITE)
+ if launchpad is not None:
+ launchpad.credentials.save_to_path(consumer_credentials_path)
+ os.chmod(
+ consumer_credentials_path, stat.S_IREAD | stat.S_IWRITE)

It's always much more secure to create the path secure (by passing a third parameter to os.open) than to chmod it later: the app may be interrupted, someone may grab it in the race window, etc.

Otherwise looks good.