Code review comment for lp:~dimitern/juju-core/330-general-improvements

Revision history for this message
John A Meinel (jameinel) wrote :

overall LGTM, though I did have some comments.

https://codereview.appspot.com/72860045/diff/2/environs/open.go
File environs/open.go (right):

https://codereview.appspot.com/72860045/diff/2/environs/open.go#newcode93
environs/open.go:93: if source != ConfigFromInfo {
On 2014/03/11 04:43:22, axw wrote:
> Is this right? You're changing behaviour here; the old code only
updated the
> error if the existing error was non-nil.

So I think this means that if an environ was bootstrapped with 1.14 then
you wouldn't have the .jenv. However, we don't need to preserve compat
with 1.14 if it makes things cleaner.
1.16+ should have always created a .jenv when it bootstrapped.

https://codereview.appspot.com/72860045/diff/2/errors/errors.go
File errors/errors.go (right):

https://codereview.appspot.com/72860045/diff/2/errors/errors.go#newcode6
errors/errors.go:6: import "fmt"
I actually prefer the other form.
Because as soon as we need 2 imports, we want to do it anyway, and that
avoids having the noise of what actually changed confused with the extra
lines.

Certainly not worth going around again, just wanted to give my feeling
that multiline imports are still reasonable to use when you only have
one.

https://codereview.appspot.com/72860045/diff/2/juju/api.go
File juju/api.go (right):

https://codereview.appspot.com/72860045/diff/2/juju/api.go#newcode107
juju/api.go:107: }
I didn't see a change in apiconn_test.go that would actually exercise
this. Can you add one so we don't break it by accident?

https://codereview.appspot.com/72860045/

« Back to merge proposal