Code review - Issue 14619045: state: make Addresses use Machine.Addresseshttps://codereview.appspot.com/2013-10-31T12:13:32+00:00rietveld
Message from unknown
2013-10-17T17:53:18+00:00rogurn:md5:afa6bf4246532614ed5907d96808e679
Message from unknown
2013-10-17T17:58:25+00:00rogurn:md5:f4420d80bc1505f997bb9d992757e299
Message from rogpeppe@gmail.com
2013-10-17T17:58:40+00:00rogurn:md5:f151ef80422953187463e93577f55d5e
Please take a look.
Message from nate.finch@gmail.com
2013-10-18T13:07:33+00:00natefinchurn:md5:a9ac0b8c65e711b9f874d7960e0b8c42
LGTM with a couple very minor nitpicks.
https://codereview.appspot.com/14619045/diff/4001/state/apiserver/common/addresses.go
File state/apiserver/common/addresses.go (right):
https://codereview.appspot.com/14619045/diff/4001/state/apiserver/common/addresses.go#newcode12
state/apiserver/common/addresses.go:12: // It is implemented by state.State.
Not a fan of having the comment on the interface tell you what implements the interface. It's way too far from the implementation, and therefor easy for it to get out of date. Also, it just seems contrary the idea of implicit implementations.
https://codereview.appspot.com/14619045/diff/4001/state/apiserver/common/addresses.go#newcode23
state/apiserver/common/addresses.go:23: st AddressAndCertGetter
st is a terrible name for an AddressAndCertGetter. I get that the only thing that implements it is state, but that's information external to the type, and makes the code in this file more difficult to read.
https://codereview.appspot.com/14619045/diff/4001/state/apiserver/common/addresses.go#newcode28
state/apiserver/common/addresses.go:28: func NewAddresser(st AddressAndCertGetter) *Addresser {
as above
https://codereview.appspot.com/14619045/diff/4001/state/apiserver/provisioner/provisioner_test.go
File state/apiserver/provisioner/provisioner_test.go (right):
https://codereview.appspot.com/14619045/diff/4001/state/apiserver/provisioner/provisioner_test.go#newcode50
state/apiserver/provisioner/provisioner_test.go:50: for i := 1; i < 3; i++ {
can you just make this 0 to 2 instead of 1 to 3? confuses the heck out of me as-is.
Message from unknown
2013-10-18T14:09:03+00:00rogurn:md5:984413a6f3dcbaa8f5a464e8031e24b2
Message from rogpeppe@gmail.com
2013-10-18T14:09:05+00:00rogurn:md5:ddd383854e08a5c6483aa7a68f58b393
Please take a look.
https://codereview.appspot.com/14619045/diff/4001/state/apiserver/common/addresses.go
File state/apiserver/common/addresses.go (right):
https://codereview.appspot.com/14619045/diff/4001/state/apiserver/common/addresses.go#newcode12
state/apiserver/common/addresses.go:12: // It is implemented by state.State.
On 2013/10/18 13:07:33, nate.finch wrote:
> Not a fan of having the comment on the interface tell you what implements the
> interface. It's way too far from the implementation, and therefor easy for it to
> get out of date. Also, it just seems contrary the idea of implicit
> implementations.
Done.
https://codereview.appspot.com/14619045/diff/4001/state/apiserver/common/addresses.go#newcode23
state/apiserver/common/addresses.go:23: st AddressAndCertGetter
On 2013/10/18 13:07:33, nate.finch wrote:
> st is a terrible name for an AddressAndCertGetter. I get that the only thing
> that implements it is state, but that's information external to the type, and
> makes the code in this file more difficult to read.
Done.
https://codereview.appspot.com/14619045/diff/4001/state/apiserver/common/addresses.go#newcode28
state/apiserver/common/addresses.go:28: func NewAddresser(st AddressAndCertGetter) *Addresser {
On 2013/10/18 13:07:33, nate.finch wrote:
> as above
Done.
https://codereview.appspot.com/14619045/diff/4001/state/apiserver/provisioner/provisioner_test.go
File state/apiserver/provisioner/provisioner_test.go (right):
https://codereview.appspot.com/14619045/diff/4001/state/apiserver/provisioner/provisioner_test.go#newcode50
state/apiserver/provisioner/provisioner_test.go:50: for i := 1; i < 3; i++ {
On 2013/10/18 13:07:33, nate.finch wrote:
> can you just make this 0 to 2 instead of 1 to 3? confuses the heck out of me
> as-is.
Done.
Message from unknown
2013-10-31T12:13:28+00:00rogurn:md5:add212dd2388afaf403c04dcf2244021
Message from rogpeppe@gmail.com
2013-10-31T12:13:32+00:00rogurn:md5:dd170a76f806162e7a1138adeba8e947
Please take a look.