Interface embedding

[Remko Tronçon]

The Google Cloud Go library contains the following code (See https://github.com/googleapis/google-cloud-go/blob/38a040e213cc8af5b01b3afe422481493f54382f/datastore/client.go#L36 )

    // datastoreClient is a wrapper for the pb.DatastoreClient
    type datastoreClient struct {
// Embed so we still implement the DatastoreClient interface,
// if the interface adds more methods.
pb.DatastoreClient
c  pb.DatastoreClient

    }

    func newDatastoreClient() pb.DatastoreClient {
return &datastoreClient{ c: pb.NewDatastoreClient() }
    }

    func (dc *datastoreClient) Lookup() {

    // ...

What could be the reason for both an anonymous and a named structure implementation, without initializing the anonymous one? As I understand it, this means the compiler no longer complains about missing methods from the `pb.DatastoreClient` interface, but it would crash with a nil pointer error when they are called?

thanks,

Remko

[roger peppe]

--
You received this message because you are subscribed to the Google Groups "golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/dfe1b583-5d79-4264-9a04-751ced59b73fn%40googlegroups.com.

The comment gives at least one reason, but I'm not sure I'm entirely convinced it's worthwhile.

I guess the reasoning could go something like:

1. We want to wrap the datastore client with our own methods, providing specific functionality.

2. We definitely don't want any of the wrapper client's methods to be called - we'd prefer to panic in that case.

3. We want to be resilient when other methods are added to the DatastoreClient interface so that the code will still compile when that happens.

Points 2 would argue against just embedding the wrapped client directly: suppose that technique was used and a new method was added - the embedding would cause the new method to be called on the wrapped client. In this particular scenario, one could argue that the wrapper type is unexported, so the package has full control of which methods are called, so this is just defensive programming.

However in fact, there appears to be no need (*) for the datastoreClient type to implement pb.DatastoreClient at all - IMHO it would all be simpler and more obvious if the code was just using that type directly, perhaps defining its own interface for testing purposes.

(*) a fact which is easy to check by removing the embedded interface, renaming a method and checking that it still compiles.

[Remko Tronçon]

3. We want to be resilient when other methods are added to the DatastoreClient interface so that the code will still compile when that happens.

That was my guess too: that it's a tradeoff between safety and future compatibility. This allows your library users to use newer versions of the protobuf dependency, without forcing the library to be updated by the library author or the library dependency to be bumped by the user. On the other hand, so would embedding the interface anonymously, but maybe the failures are more subtle if you accidentally call a method you didn't account for (e.g., in this case, no headers are passed).

However in fact, there appears to be no need (*) for the datastoreClient type to implement pb.DatastoreClient at all

The unit tests depend on this being an interface that can be faked; I guess this avoids having to create your own interface for the subset of methods you're calling on the client. 

It would also enable library users to pass their own client, although this doesn't seem to be possible using the API.

thanks,

Remko