Code review comment for lp:~fwereade/pyjuju/go-place-unit
Revision history for this message
| William Reade (fwereade) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
0e1f616
(Get the code!)
Please take a look.
https:/
File state/unit.go (right):
https:/
state/unit.go:26: type PlacementPolicy string
On 2012/06/05 19:58:08, niemeyer wrote:
> Can we name this as AssignmentPolicy? I'd also be equally happy to go
to the
> other direction and name methods as PlaceInMachine etc.
Assignment SGTM
https:/
state/unit.go:30: PlaceUnassigned PlacementPolicy = "unassigned"
On 2012/06/05 19:58:08, niemeyer wrote:
> It'd be nice to have a sentence on top of each explaining their
meaning.
Done.
https:/
state/unit.go:241: func (u *Unit) Place(policy PlacementPolicy) (err
error) {
On 2012/06/05 19:58:08, niemeyer wrote:
> It's awkward to have this as a method of the unit. Not only is it
using the unit
> just through its API, but it's also digging through and *creating
machines*
> silently.
> IMO, this is better suited for a utility function such as:
> func AssignUnit(st *State, u *Unit, policy AssignmentPolicy)
> (or Place/Placement, depending on which side you prefer)
> It'd be useful as well to document the fact it not only assigns the
unit but may
> also create a brand new machine to accomplish the task it was given.
Done.
https:/
state/unit.go:250: case noUnusedMachines:
On 2012/06/05 19:58:08, niemeyer wrote:
> if err != noUnusedMachines {
> return err
> }
D'oh! Done.
https:/