Hmm, slightly behind on my email. So further to my earlier message, I'm now happy that this isn't going to cause a *huge* amount of disruption for our users, and the benefit of cleaning up the entity base class and generalising the configuration code exceeds that cost.
Richard. On 9 September 2014 23:51, Aled Sage <[email protected]> wrote: > 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`. >>> >>> >
