Code review comment for lp:~hazmat/pyjuju/local-network

Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :

Very nice, +1 with some simple bits only.

[1]

+ subprocess.check_call(["virsh", "net-start", name])

It occurred to me that an error on these calls will pretty
often crash the whole application, because we won't catch
the exception coming out of it. Should we be trying to be
more careful about handling error situations and reporting
them instead of letting them go through, or are such errors
indeed supposed to terminate the app?

[2]

+def list_networks():

Having some sample output of the program in a comment would
make the logic much easier to follow.

[3]

+ networks_raw = filter(
+ None, [n.strip() for n in output.split("\n")][2:])
+ for name, state, auto in [n.split() for n in networks_raw]:
+ networks[name] = (state == "active") and True or False

I agree regarding clarity. E.g.:

# The first two lines contain headers
output_lines = output.splitlines()[2:]
for line in output_lines:
    if not line.strip():
        continue
    name, state, auto = line.split()
    networks[name] = (state == "active")

[4]

+def get_network_attributes(name):

Sample output would be useful as well. Would give a better idea
of what the code is looking after (e.g. why e.g. the ip is
processed the way it is).

review: Approve

« Back to merge proposal