Code review comment for lp:~rogpeppe/juju-core/562-lose-testbase

Revision history for this message
Wayne Witzel III (wwitzel3) wrote :

On 2014/05/30 16:30:53, wwitzel3 wrote:
> My feeling is, I don't care where and how things are aliased, but it
should be
> consistent so that when I am looking at a file and I see "coretesting"
and the
> next file I see "testing" .. I shouldn't have to think that might be
> juju-core/testing, just sans alias.

> Also I'm -1 with respect to alias of the standard lib testing to
stdtesting.
> That just inserts unneeded convention in to our code. We can easily
alias our
> other testing packages and not have to remember that stdtesting is
really just
> testing.

> I think we should try to use the aliases coretesting, gitjujutesting,
> statetesting, jujutesting, etc.. even if they aren't conflicting with
any other
> imported packages. That consistency will make our code a whole lot
easier to
> follow IMO.

> https://codereview.appspot.com/99670045/diff/40001/cmd/package_test.go
> File cmd/package_test.go (right):

https://codereview.appspot.com/99670045/diff/40001/cmd/package_test.go#newcode7
> cmd/package_test.go:7: stdtesting "testing"
> Why alias the standard library testing package?

https://codereview.appspot.com/99670045/diff/40001/cmd/supercommand_test.go
> File cmd/supercommand_test.go (right):

https://codereview.appspot.com/99670045/diff/40001/cmd/supercommand_test.go#newcode16
> cmd/supercommand_test.go:16: "launchpad.net/juju-core/testing"
> Why not alias this to coretesting?

https://codereview.appspot.com/99670045/diff/40001/environs/sync/sync_test.go
> File environs/sync/sync_test.go (right):

https://codereview.appspot.com/99670045/diff/40001/environs/sync/sync_test.go#newcode15
> environs/sync/sync_test.go:15: "testing"
> Same vein. Here we don't alias to stdtesting.

https://codereview.appspot.com/99670045/diff/40001/environs/sync/sync_test.go#newcode32
> environs/sync/sync_test.go:32: coretesting
"launchpad.net/juju-core/testing"
> Again, here we do alias to coretesting.

And by "how" I meant, naming wise.

https://codereview.appspot.com/99670045/

« Back to merge proposal