Code review comment for lp:~bcsaller/pyjuju/ensemble-status

Looks very nice.

[1]
pep8isms
control/status.py

line 81: help="An optional filename to output the result to",
line over 80

line 124: EnvironmentsConfigError undefined

line 233: two blank lines before func

line 354: extra blank line at the end of the file

[2]
    renderer = getattr(globals(), "render_%s" % options.format, render_json)

I'm not a big fan of grabbing things of globals, it feels a bit
implicit. It would be nice to toss the renderers into a class,
dictionary, or instance as namespace, and then just use getattr
against the namespace.

[3]
def encode_state(o):

Its not entirely clear what this is doing, or why it would get a
deferred instead of an object. it would be nice to have doc strings,
and in general its better to spell out variable names, then use
abbreviations.

[4]
import pydot

python-pydot probably should get added to the debian/control as dep, or at least a suggestion.

[5]

machine.py line 81: machine_id = int(machine_id[-10:])
there's an _internal_id_to_id function in the same module that should be used for this.

[6]
relation.py line 264: get_services

it doesn't feel right to have a method to return all the services
associated to a servicerelationstate, as its effectivly returning
itself as well. The relation stuff feels a bit unwieldy in this
respect, but it would be nice to have this sort of method on the
relation state manager. i'm not entirely sure what that should like
though.

[7] unrelated and for the future enhancement of status in a separate
branch, but i just added code to expose at least the machine provider
dns names on the provider machines, as well an accessor api to get
them individual (in addition to the existing bulk api).