Code review comment for lp:~broder/ubuntu-dev-tools/fix-785854

Hi Evan,

I totally see why you've added this, and changed the code to use it. Thanks for making ubuntu-dev-tools better! Of course, once we migrate u-d-t to Python 3.2 this change won't be necessary <wink>.

I'm a little concerned that what you've written isn't entirely API compatible with Python's version. Python's subprocess.Popen is a class while ubuntutools.subprocess.Popen is a function. I have no idea whether people actually try to subclass subprocess.Popen, and within u-d-t it's easy enough to guarantee this, but is it possible there are would be external clients that could be broken by this? It makes me uncomfortable that your docs claim API compatibility when it's really not.

Two thoughts: could this be implemented as a subprocess.Popen subclass?

Or, would it make sense to just use a different API rather than promoting something that looks like stdlib subprocess.Popen but isn't quite? The way you have it does mean that existing code in u-d-t doesn't have to change, except for the import statement. I'm not sure whether that's good or not ;).

You might also call inspect.getargspec() at module scope and then not redefine, but simply expose the stdlib subprocess functions directly in ubuntutools.subprocess.Popen.

Anyway, just some thoughts. At the least, please do indicate in the docs about this difference. I'm happy to chat about alternatives in IRC if you want.