Code review comment for lp:~justin-fathomdb/nova/check-subprocess-exit-code

Revision history for this message
justinsb (justin-fathomdb) wrote :

Agree that the pip comment should be in a different branch, as probably should the --fail argument to curl; just trying to minimize the overhead involved in patch submission.

On the changes to the twisted code, there were intended to fix the problems with checking stderr & exit code, that were documented in the code itself. It also felt cleaner to have one class than two, particularly as some methods in the base class appeared to be broken, but were never called as the derived class overrides those method. I'm happy to reapply my fixes against a branch if you want to submit something more along your lines of thinking.

The other comments seem to be about formatting. Can you please point me to the formatting rules we're using? Ideally (from my point of view) we could just all agree on a vim configuration (or some other editor) and post that for Python noobs such as myself.

Not sure I agree on use of camel case because Twisted does it, given that the rest of the code seems to use underscores, and Twisted might well be ripped out anyway. But this is the sort of thing that will be covered by the project formatting rules, so we don't need to debate it :-)

If someone can point me to the formatting rules for vim that we're using, I'll auto-format the code and resubmit.

« Back to merge proposal