I was planning to attend the standup today, but I've been summoned to dinner at my mother in law's this evening. I'd appreciate if someone would jot down the takeaways from the discussion. Here's a few thoughts from me:
In general, good, easy to understand code. Good ratio of code to comments (and useful comments too). Some of the utility function names are meaningful in the file context, but not so much at the package level. e.g. ParentId (parent of what?), NestingLevel (nesting level of what?), TopParentId (you get the picture). To fill in the gap you need to be in the right file, or read the docstring. I'm a big believer in self-describing names. Also, maybe these functions should move to "names"? machineContainers's field names are a little opaque. I'd prefer Id->MachineId, Children->ContainerIds. Looks like the structure mirrors the DB schema though. createContainerRefOp's docstring is missing a word after the function name, which confused me (as someone who's never been through this code before). After reading the docstring for removeContainerRefOps, it's clear what it should be. Cheers, Andrew On Wed, Aug 21, 2013 at 2:59 PM, John Arbash Meinel <[email protected]>wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > Since I'm getting this out a bit late, I'm going to try and go with a > smaller file. For those that get this in time, if you can take about 1 > hour to review the file: > > state/container.go (and its associated tests, etc) > > Obviously some of the time will be spent looking up what various > objects are in other files, etc. > > We'll be talking about it during the standup time today. > > John > =:-> > -----BEGIN PGP SIGNATURE----- > Version: GnuPG v1.4.13 (Cygwin) > Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ > > iEYEARECAAYFAlIUZTQACgkQJdeBCYSNAANsMACgvY57WJ3Smq5d6NxFfA3X3vdR > 7dUAn1ZW7dE+2WqOYz1wr+VRhDP4YehV > =mZyj > -----END PGP SIGNATURE----- > > -- > Juju-dev mailing list > [email protected] > Modify settings or unsubscribe at: > https://lists.ubuntu.com/mailman/listinfo/juju-dev >
-- Juju-dev mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
