Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(14286)

Issue 99290043: state/apiserver/common: RegisterStandardFacade

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 11 months ago by jameinel
Modified:
9 years, 11 months ago
Reviewers:
mp+219660, fwereade
Visibility:
Public.

Description

state/apiserver/common: RegisterStandardFacade So my previous patch introduced a Registry that can be used for registering and then looking up Facades. However, almost all of our existing code has methods like: func NewKeyManagerAPI( st *state.State, resources *common.Resources, authorizer common.Authorizer, ) (*KeyManagerAPI, error) { And that has the property that it is != o type FacadeFactory func( st *state.State, resources *Resources, authorizer Authorizer, id string, ) (interface{}, error) Because the return type for the latter is interface{} and the former is a concrete type (that implements that interface, of course). I could think of 3 ways to work around this, and this seemed the most tasteful. 1) Rewrite all of the New*API() functions to return interface{}, and then rewrite all of their tests to cast it back to the concrete type. I like that New* returns a concrete type, though, so I'd rather keep that. 2) Wrap all of the calls to RegisterFacade with a bespoke func() that just does the type currying, then all of the init() functions looked like: func init() { common.RegisterFacade("KeyManager", 0, func( st *state.State, resources *Resources, authorizer Authorizer, id string, ) (interface{}, error) { if id != "" { return nil, common.ErrBadId } return NewKeyManagerAPI(st, resources, authorizer), nil }) } That isn't terrible, but it is a lot of boilerplate to write. 3) Use reflect to create the boilerplate functions. Then all of the actual init lines just become: func init() { common.RegisterStandardFacade("KeyManager", 0, NewKeyManagerAPI) } So at the cost of having RegisterStandardFacade have to do reflect introspection, it gives us really nice syntax when we want to add more. (And note that when we have multiple versions of Facades, (2) would have to have a bespoke func() for each one.) I could go back to (2) if people don't like the reflect magic, but I think it does make things nicer overall. https://code.launchpad.net/~jameinel/juju-core/api-register-standard-facade/+merge/219660 Requires: https://code.launchpad.net/~jameinel/juju-core/api-facade-registry/+merge/219654 (do not edit description out of merge proposal)

Patch Set 1 #

Patch Set 2 : state/apiserver/common: RegisterStandardFacade #

Unified diffs Side-by-side diffs Delta from patch set Stats (+211 lines, -0 lines) Patch
A [revision details] View 1 1 chunk +2 lines, -0 lines 0 comments Download
A state/apiserver/common/export_test.go View 1 chunk +9 lines, -0 lines 0 comments Download
M state/apiserver/common/registry.go View 1 2 chunks +85 lines, -0 lines 0 comments Download
M state/apiserver/common/registry_test.go View 1 2 chunks +115 lines, -0 lines 0 comments Download

Messages

Total messages: 3
jameinel
Please take a look.
9 years, 11 months ago (2014-05-15 08:42:21 UTC) #1
fwereade
LGTM. The yuck of the reflection is more than balanced by the squee of the ...
9 years, 11 months ago (2014-05-16 09:59:28 UTC) #2
jameinel
9 years, 11 months ago (2014-05-16 11:10:14 UTC) #3
Please take a look.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b