Code review comment for lp:~niemeyer/pyjuju/go-schema

Thanks for the review William. Some feedback:

[0][1]

Yes, in your suggested model we have to shrink and expand the list on every iteration of the loop, while this is a simple assignment. The Python version works the same way.

[2]

Our strict long variable name policy has lead us to a position that is not entirely nice. I'm still very much for readability, and that's precisely the point in fact. Long names in tight contexts are doing more harm than good in our code base, leading to very long lines that often wrap and easily become difficult to follow.

I still think we should be careful about names, even more when these names have a wider scope, but I'd like to propose we take a different stanza in Go, and attempt to follow the practices and style used by the Go development team itself (in a similar way we use PEP8 in Python). Go also has the advantage of typing and static compilation, so it's easier to infer what something is, and harder to make mistakes.

Just for context, here is an example of long names getting in the way of readability:

    @inlineCallbacks
    def test_machine_cannot_be_removed_if_assigned(self):
        """Verify that a machine cannot be removed before being unassigned"""
        machine_state = yield self.machine_state_manager.add_machine_state()
        service_state = yield self.service_state_manager.add_service_state(
            "wordpress", self.formula_state)
        unit_state = yield service_state.add_unit_state()
        yield unit_state.assign_to_machine(machine_state)
        ex = yield self.assertFailure(
            self.machine_state_manager.remove_machine_state(machine_state.id),
            MachineStateInUse)
        self.assertEqual(ex.machine_id, 0)
        yield unit_state.unassign_from_machine()
        topology = yield self.get_topology()
        self.assertTrue(topology.has_machine("machine-0000000000"))
        removed = yield self.machine_state_manager.remove_machine_state(
            machine_state.id)
        self.assertTrue(removed)
        topology = yield self.get_topology()
        self.assertFalse(topology.has_machine("machine-0000000000"))
        # can do this multiple times
        removed = yield self.machine_state_manager.remove_machine_state(
            machine_state.id)
        self.assertFalse(removed)

[3]

I agree with you on that. I just can't work on this right now, but let's try to go into that direction.