client: public API

[anpep]

This patch, similar in spirit to #238, introduces a new public API for
extending the Pebble client. The basic idea behind this patch is the client mixin
struct embedded in commands' structs no longer holds an instance of the
Pebble-specific client.Client struct, but instead take a ClientGetter interface
that implements a Client() method. ClientGetter.Client() always returns a
Pebble-specific client.Client struct.

For applications that intend to extend Pebble, this means that either the Pebble
client or a new, application-specific client can be used. In the latter case,
the application-specific client must implement the ClientGetter interface so
that a Pebble-specific client.Client struct can always be derived by the
facilities consuming the ClientGetter interface. The easiest way to implement
this is to embed the Pebble client:

type PebbleClient = client.Client
type MyClient struct {
    *PebbleClient
}

Since the Pebble-specific client.Client is embedded, and the ClientGetter
interface requires Client() to be implemented with a pointer receiver, the
snippet above suffices to implement a client based off the Pebble-supplied one
without much hassle, and that provides lower-level facilities for communicating
with the daemon, such as DoSync(), DoAsync() and DoAsyncFull().

[benhoyt]

@anpep Would you be able to give more context and description about what problem this change is trying to solve, and how it does that? (And specifics like why ContextGetter is needed, that kind of thing.) That would be really helpful for reviewers.

[benhoyt]

I don't understand how this helps. The ClientMixin still uses the concrete client type, it's just behind a method which returns a concrete *client.Client. Can you show a small example of how this would be used to explain how it helps?

[flotter]

@anpep I did a detailed review as a patch on your proposal and sent it to you. I did it this way to test the proposal as its very difficult without the kernos side :)

[benhoyt]

I think the changes to export DoSync (and similar methods) and the addition of RequestInfo are good.

However, I don't like the ClientGetter / ClientSetter stuff at all. It seems very indirect and implicit, and hard to follow how things are being wired up. This isn't really your fault -- you're extending the existing clientMixin / clientSetter pattern, which I've always found a bit messy and hard to follow anyway: you add this clientMixin to your command struct, and then because it implements setClient (determined by type assertion at runtime), the parser knows to call it, which magically fills in the client field for the command's Execute to use.

Side notes: it's also weird that a new client is created even for commands that don't need it (though it looks like help is the only one like that right now, so perhaps a bit of a theoretical concern). I also don't like how something called a "parser" is not just parsing, but executing, the commands -- but that's really a separate, go-flags issue.

I wonder if instead of getting further entrenched in this hard-to-understand implicit wiring, we can create and wire up the client more directly. I think there are several ways we could do this, but I think the most obvious is just to create the client in the command's Execute method. This is how run works with the daemon: the Daemon is created inside Execute (it's a little nested, but it is direct: Execute > run > runDaemon > daemon.New).

For example, consider pebble changes:

func (c *cmdChanges) Execute(args []string) error {
	// These few lines are the new code:
	cl, err := client.New(clientConfigFromEnv())
	if err != nil {
		return err
	}
	defer maybePresentWarnings(cl)

	// This is existing code:
	changes, err := queryChanges(cl, &opts)
	if err != nil {
		return err
	}
	// ...
}

The clientConfigFromEnv function would be like getEnvPaths, but build a *client.Config from environment variables. I realise this would need to be overridden by another program like Termus that used Pebble. But there are various simple ways to do that, for example adding EnvDir and EnvSocket to the personality struct to allow you to change the names of the environment variables, and default them to PEBBLE and PEBBLE_SOCKET.

The maybePresentWarnings is something that's currently in Run that most commands do at the end -- if there were warnings, they show WARNING: There are %d new warnings. See 'pebble warnings'.. So we need to keep that (and may need to make it exported for Termus et al).

This approach means a few lines of boilerplate in every Execute method, but it's clear and explicit, and it gets rid of the clientMixin / setClient messiness, as well as allowing you to create whatever type of client you want on Termus Execute methods.

What do you think? I have Tuesday evening meetings (EU morning time) today anyway, so I'm going to try to schedule a meeting today for us to chat further.

[anpep]

Closing because this approach was ruled out (: