Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Don't return typed nil interface values #46
+54
−0
Conversation
dimitern
reviewed
Apr 25, 2016
| @@ -19,6 +19,11 @@ type deviceSuite struct { | ||
| var _ = gc.Suite(&deviceSuite{}) | ||
| +func (*deviceSuite) TestNilZone(c *gc.C) { | ||
| + var empty device | ||
| + c.Check(empty.Zone() == nil, jc.IsTrue) |
dimitern
Apr 25, 2016
Contributor
Or alternatively:
var zeroDevice device
c.Check(zeroDevice.Zone(), gc.IsNil)
babbageclunk
Apr 25, 2016
Member
I tried that first, but it uses reflection so didn't fail before I fixed the problem.
dimitern
reviewed
Apr 25, 2016
| @@ -19,6 +19,11 @@ type interfaceSuite struct { | ||
| var _ = gc.Suite(&interfaceSuite{}) | ||
| +func (*interfaceSuite) TestNilVLAN(c *gc.C) { |
dimitern
reviewed
Apr 25, 2016
| @@ -13,6 +13,11 @@ type linkSuite struct{} | ||
| var _ = gc.Suite(&linkSuite{}) | ||
| +func (*linkSuite) TestNilSubnet(c *gc.C) { |
dimitern
reviewed
Apr 25, 2016
| @@ -19,6 +19,14 @@ type machineSuite struct { | ||
| var _ = gc.Suite(&machineSuite{}) | ||
| +func (*machineSuite) TestNilGetters(c *gc.C) { |
dimitern
reviewed
Apr 25, 2016
| @@ -13,6 +13,11 @@ type partitionSuite struct{} | ||
| var _ = gc.Suite(&partitionSuite{}) | ||
| +func (*partitionSuite) TestNilFileSystem(c *gc.C) { |
dimitern
reviewed
Apr 25, 2016
| @@ -13,6 +13,11 @@ type subnetSuite struct{} | ||
| var _ = gc.Suite(&subnetSuite{}) | ||
| +func (*subnetSuite) TestNilVLAN(c *gc.C) { |
|
LGTM, apart from a few suggestions for tests simplifications. |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju-gomaasapi |
jujubot
merged commit 9ec917c
into
juju:master
Apr 25, 2016
babbageclunk
deleted the
babbageclunk:nil-interfaces
branch
Apr 25, 2016
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
babbageclunk commentedApr 22, 2016
•
Edited 1 time
-
babbageclunk
Apr 22, 2016
Fix crash in Juju caused by typed nil Link.
https://bugs.launchpad.net/juju-core/+bug/1573659
The line immediately before the panic checked for nil, but because the type pointer is set on the Link it wasn't equal to nil.
From reading about this it seems like methods that return interfaces shouldn't implicitly return nil values, they should return nil explicitly, otherwise the result will be a typed nil. Checking for a typed nil on the client side is awkward - if the underlying type is known and accessible you could try a type assertion, otherwise it requires reflection.
https://golang.org/doc/faq#nil_error
(About errors, but applies to any interface.)