Hi Aled, Having a consistent set of methods for each type - good The code for managing configuration to be in the base type and shared for all implementations - good Putting all the configuration into a separate class - not sure
What's the impact on our users, and what are the benefits for them? I can see that architecturally it's a good idea (a class should only do one thing...) but I'm not yet convinced that on practical levels, the benefits outweigh the costs of the change? And I think there is a better name than `getConfigSupport()` - maybe just `configuration()...` </bikeshedding> Richard. On 22 August 2014 12:57, Aled Sage <[email protected]> wrote: > Hi all, > > I'd like us to make the config methods the same on Entity, Location, Policy > and Enricher. > > As well as making things easier to understand/use, it will also allow for > more code reuse (i.e. deleting code duplication) by pushing this up into the > super-type BrooklynObject. > > --- > PROPOSAL > We add `BrooklynObject.getConfigSupport()` which returns `ConfigSupport`, > which has a rationalised set of methods for accessing + setting config. > > We deprecate (most of?) the methods on Entity etc. > > We stop needing to use `EntityLocal.setConfig`, which currently causes a > whole load of ugly casts in our code! > > --- > JUSTIFICATION > > * It's confusing (and results in more code) that the methods are > different on Entity versus Policy etc. > * We have nine methods on Entity/EntityLocal for getConfig, setConfig > or getConfigRaw. > That number easily justifies grouping them in a class with something > like `getConfigSupport`. > On Location, we have size methods (with an overlap of two on Entity!) > > --- > QUESTION: > Which top-level methods on Entity do we keep? > Is `entity.getConfig(configKey)` called so often that we don't want to force > `entity.getConfigSupport().getConfig(configKey)`? > Any others? > > --- > PROPOSAL PART TWO > We could do the same for attributes/sensors (which are currently only > available on Entities, so that is where we'd put `getAttributeSupport`). > Currently one casts to be able to call `EntityLocal.setAttribute`. > > Alternatively, `setAttribute` could be moved to be a top-level method on > `Entity`. > >
