Hi Richard,
Good points. I prefer your name of `configuration()`.
Impact on users would be that authors of the more complicated entities
would need to refactor their usage of `getAllConfig()`,
`getAllConfigBag()`, etc. I view some of that as inevitable anyway, as
the API needs a cleanup and we need to make entity+location+policy APIs
consistent for the long-term benefit of our users. Better to do that
sooner rather than later.
Most "simple" entities (e.g. for Tomcat and JBoss app server) just call
`getConfig(ConfigKey)` and occassionally `setConfig(ConfigKey, val)`. We
could leave those two duplicated on Entity as conveniences, with javadoc
that it delegates to `configuration().getConfig()` etc.
As we are moving towards less Java for most new entities (and will
slowly refactoring existing ones), I'm not too worried about the
refactoring involved for power users.
(But that's a different discussion thread).
---
So the big question I think is: will this make the interface nicer to
use + support (given we're changing some of entity/location/policy anyway)?
Aled
On 25/08/2014 10:54, Richard Downer wrote:
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`.
