Code review comment for lp:~thumper/juju-core/make-syslog-port-configurable

Revision history for this message
Ian Booth (wallyworld) wrote :

https://codereview.appspot.com/29990045/diff/1/worker/deployer/addresser.go
> File worker/deployer/addresser.go (right):

https://codereview.appspot.com/29990045/diff/1/worker/deployer/addresser.go#newcode15
> worker/deployer/addresser.go:15:
> On 2013/11/22 00:46:52, wallyworld wrote:
> > On 2013/11/21 05:17:42, axw wrote:
> > > This seems like the wrong place to be talking about the API. This
should
> just
> > be
> > > about domain objects, I think.
> >
> > +1
> > params.ServerAddressesResults does not belong here. No api params
structs
> should
> > escape the api infrastructure

> OK, I disagree here on two fronts.

> Firstly this interface should be renamed to have something to do with
the API.
> It defines the API methods that it calls.

> Secondly there is already precedence for exporting api/params
structures as
> return parameters for client api method calls.
> I don't see anything wrong with that as the api/params package is
defined to
> have no other dependencies, and is just a holding package for
structures. It
> seems stupid to redefine it just because.

> This interface should be both renamed, and have the first two methods
removed,
> as they are no longer needed.

Might be easier to discuss via a hangout. I have been tending to view
the params.* structs as internal, on-the-wire, representations of the
data to pass from client to server. And use of these outside of the api
becomes a layering violation. Perhaps I'm wrong

https://codereview.appspot.com/29990045/

« Back to merge proposal