Code review comment for lp:~jameinel/juju-core/reflect-only-facades

I realise this is the direction you have decided to go, but before you do,
I think it is worth considering the advantages that a strictly static set
of exported methods provides. Specifically it allows the possibility of
allowing clients to introspect rpc method types without invoking the
methods, and other code to see all the methods exported, for example to
automatically generate documentation for the entire API or to generate
client entry points similarly (both of which I have done in the past).

I personally think that it is a pity we are to lose these possibilities,
when there is a reasonable versioning alternative available (we could just
include the version number in the top level method names).
On 8 May 2014 12:36, "John A Meinel" <email address hidden> wrote:

> Reviewers: mp+218764_code.launchpad.net,
>
> Message:
> Please take a look.
>
> Description:
> rpc: expose concrete types
>
> This changes the RPC package a bit so that if you are declaring
> RootMethod that returns 'interface{}' it can find the concrete type that
> is being returned and expose the methods on that type (instead of
> exposing 0 methods from interface{}).
>
> This is a step along the way to supporting passing versions for Facades
> in the "id" slot. We currently let you pass the value, but the concrete
> type that is returned defines all of the Methods that could be
> available. Which means that if you add a new Method in V2 of your API,
> you would suddenly have that exposed in the V1 of your API. Similarly
> you couldn't *actually* ever change the interface for a give method,
> because then you couldn't call it one way for V1 and a different way for
> V2.
>
> There is a fair bit more to be done before we have actual API
> versioning, but this is a step along that path and it seemed cohesive
> enough to review on its own.
>
>
> https://code.launchpad.net/~jameinel/juju-core/reflect-only-facades/+merge/218764
>
> (do not edit description out of merge proposal)
>
>
> Please review this at https://codereview.appspot.com/98090043/
>
> Affected files (+290, -61 lines):
> A [revision details]
> M rpc/reflect_test.go
> M rpc/rpc_test.go
> M rpc/rpcreflect/type.go
> M rpc/rpcreflect/value.go
> M rpc/server.go
>
>
>
> --
>
> https://code.launchpad.net/~jameinel/juju-core/reflect-only-facades/+merge/218764
> Your team juju hackers is requested to review the proposed merge of
> lp:~jameinel/juju-core/reflect-only-facades into lp:juju-core.
>