+1, sorry for the late answer, did not had much time to review your branch before
On Wed, Oct 31, 2012 at 1:17 PM, Andreas Jonsson <[email protected]> wrote: > Hi, > > Obviously, I should vote +1 myself as well. So, we have 2 +1 and 1 +0. > Does anyone else have an opinion on this? Maybe Thomas? > > /Andreas > > 2012-10-29 19:31, Sergiu Dumitriu skrev: >> On 10/28/2012 04:43 PM, Andreas Jonsson wrote: >>> Hi Vincent, >>> >>> 2012-10-27 19:13, Vincent Massol skrev: >>>> Hi Andreas, >>>> >>>> On Oct 26, 2012, at 2:07 PM, Andreas Jonsson <[email protected]> wrote: >>>> >>>>> Hi Everyone, >>>>> >>>>> I would like you to vote on merging the feature-execution-context >>>>> branches of commons and platform before the release of 4.3M2: >>>>> >>>>> https://github.com/xwiki/xwiki-commons/compare/master...feature-execution-context-metadata >>>>> https://github.com/xwiki/xwiki-platform/compare/master...feature-execution-context-metadata >>>>> >>>>> Explanation of the feature: >>>>> >>>>> The execution context is a simple map that binds a string to an object. >>>>> This can be compared to how variables are handled by scripting languages >>>>> such as bash, where an assignment brings the variable into existance: >>>>> >>>>> $ my_variable="some value" >>>>> >>>>> In the case of the execution context, the syntax is: >>>>> >>>>> context.setProperty("my_variable", "some value"); >>>>> >>>>> This feature is about adding property declarations, where a property can >>>>> be associated with attributes that controls how the execution context >>>>> and execution context manager handles the property. The general idea >>>>> can, once again, be compared to bash and how it is possible to declare >>>>> variables there: >>>>> >>>>> $ declare -ru my_variable="read only, forced upper case" >>>>> >>>>> Of course, the set of attributes that are interesting to us is different >>>>> from bash. Currently the feature branch have support for declaring >>>>> properties with these attributes: >>>>> >>>>> * Final - The value may not be updated within the the execution >>>>> context. Default: false. >>>>> >>>>> * Inherited - The property will be inherited from the current >>>>> context when replacing the execution context within the current scope. >>>>> Default: false. >>>>> * Cloned value - Also clone the value when the execution context is >>>>> cloned. Default: false. >>>> ATM I believe that all our properties are "cloned" since when we clone the >>>> context they are there. Won't this cause a backward compat issue? >>>> Shouldn't it be true by default? >>> No, it is not actually a compatibility issue, because the clone method >>> in the execution context manager doesn't actually clone the execution >>> context. This option is therefore only used when copying the properties >>> when inheriting the parent context, which is a new feature. >>> >>> But having said that, it is possible that we should enable cloning by >>> default. My thoughts on this is only that we cannot know if the the >>> value can be cloned, which would be a slight complication. >>> >>> >>>>> * Type - The class of the value, for typechecking when setting >>>>> the value. Default: null (unchecked). >>>>> >>>>> * Non-null - The value may not be null, checked when setting the >>>>> value. Default: false. >>>>> >>>>> Example declaration: >>>>> >>>>> ExecutionContextProperty property = new >>>>> ExecutionContextProperty("my_variable"); >>>>> property.setValue("some value"); >>>>> property.setType(String.class); >>>>> property.setInherited(true); >>>>> context.declareProperty(property); >>>> The API is very verbose just to declare one property… IMO it would be nice >>>> to have something more compact. >>> Ok. To make the API more compact, I changed it to use a builder pattern >>> for declaring properties, for example: >>> >>> context.newProperty("key").type(String.class).nonNull().initial("value").declare(); >>> >>> >>>> As I've already commented I'd personally have preferred that the >>>> ExecutionContextProperty and properties in general be immutable and set >>>> with one call to context.setProperty(key, ECP) (i.e. no need to declare >>>> IMO). I find it more straightforward and prevents forgetting to call >>>> declareProperty, which becomes really critical to get the right behavior >>>> you want. >>>> >>> Let's see if I understand you correctly. First, I would say that >>> setting a property with attributes (i.e., setProperty(key, ECP)) is >>> precisely what I mean by 'declaring' a property. You are just using a >>> different method name. So, I'm taking the liberty to continue to use >>> the word 'declaring' to denote 'setting a property with attributes'. >>> >>> With this in mind, do you suggest that we forbid updating property >>> values altogether (i.e., the same as declaring them final by default >>> with no option to not declare them final)? Also, do you suggest >>> deprecating 'ExecutionContext.setValue(String, Object)' to disallow >>> implicit declarations altogether? >>> >>> The ability to actually replace the value object seems very useful to >>> me. A simple use case is that of keeping a boolean flag in the context: >>> >>> context.newProperty("flag").type(Boolean.class).nonNull() >>> .initial((Boolean) false).declare(); >>> >>> // ... >>> >>> if (! ((Boolean) context.getProperty("flag"))) { >>> context.setProperty("flag", (Boolean) true); >>> // ... >>> } >>> >>> As of implicit declarations, I have allowed them only to maintain >>> backwards compatiblity. I would be in favour of disallowing them. >>> >>> >>>>> The property value may be updated and the property may be removed and >>>>> redeclared, unless declared 'final': >>>>> >>>>> context.setProperty("my_variable", "some new value"); >>>>> context.removeProperty("my_variable"); >>>> In general immutability is better in term of performance/implementation. >>>> >>>>> Additional attributes may be added later. This feature is also >>>>> backwards compliant, in that implicit declaration by just setting a >>>>> value is allowed. (Although we might want to make explicit declarations >>>>> mandatory in the future.) >>>>> >>>>> Within the scope of a request, or the life time of a thread executing >>>>> some administrative task (e.g., the lucene index updater) the basic life >>>>> cycle of the execution context is the following: >>>>> >>>>> @Inject >>>>> Execution execution; >>>> No need for this injection in your code below :) >>>> >>>>> @Inject >>>>> ExecutionContextManager ecm; >>>>> >>>>> ExecutionContext context = new ExecutionContext(); >>>>> ecm.initialize(context); >>>>> try { >>>>> // Do work >>>>> } finally { >>>>> ecm.removeContext(); >>>>> } >>>>> >>>>> Within the life cycle of the "root" execution context, we may push a new >>>>> execution context, which may either be a clean context, or a clone of >>>>> the current context: >>>>> >>>>> // Pushing a clone >>>>> >>>>> ExecutionContext context = ecm.clone(execution.getContext()); >>>> <brainstorming mode> >>>> Actually I wonder why we have both Execution and ECM. Shouldn't we have >>>> just one? >>> Yes, the execution context manager seems redundant. >>> >>>> Also I wonder why we have to call ecm.clone() instead of implementing >>>> clone() in ExecutionContext so that we would have: >>>> >>>> ExecutionContext context = execution.getContext(); >>>> execution.pushContext(context.clone()) >>> Given that the method ECM.clone(EC) doesn't actually clone anything, >>> particularily not the execution context given as argument, I'd say that >>> it should be deprecated in favor of making the execution context itself >>> cloneable. >>> >>> >>> Best Regards, >>> >>> /Andreas >>> >>> >>> >>>> And to create an EC: >>>> ExecutionContext ec = execution.createContext(); >>>> >>>> Ok maybe we would have a hard time with backward compat with this but for >>>> the sake of the discussion, would that be something better? >>>> </brainstorming mode> >>>> >>>>> execution.pushContext(context); >>>>> try { >>>>> // Do work >>>>> } finally { >>>>> execution.popContext(); >>>>> } >>>>> >>>>> // Pushing a clean context >>>>> >>>>> ExecutionContext context = new ExecutionContext(); >>>>> execution.pushContext(context); >>>>> try { >>>>> // Do work >>>>> } finally { >>>>> execution.popContext(); >>>>> } >>>>> >>>>> Component authors that needs to place a value in the execution context >>>>> provides an initializer that declares the variable and sets an initial >>>>> value. >>>>> >>>>> The attributes 'final', 'cloned value', and 'inherited' lets component >>>>> authors control how the value is managed during the lifecycle of the >>>>> root execution context. >>>>> >>>>> The attributes 'type' and 'non-null' provides some runtime assertions to >>>>> catch some errors earlier. >>>>> >>>>> So to summarize, this feature: >>>>> >>>>> 1. is a convenient mechanism for managing properties in the execution >>>>> context, >>>>> >>>>> 2. provides some validation of the property values, >>>>> >>>>> 3. improves performance slightly by avoiding unnecessary cloning. >>>>> >>>>> For more information, see the proposal thread: >>>>> >>>>> http://xwiki.475771.n2.nabble.com/PROPOSAL-Execution-context-property-declarations-and-property-metadata-attributes-td7581766.html >>>> +0 from me after we agree on the items above. >>>> >>>> I'd really like to get feedback from other committers before you push this >>>> since it's a really critical change (the Execution/EC is key and is going >>>> to become even more and more important as we move away from XWikiContext). >> I agree with the current state of the code, +1 for merge. >> > > _______________________________________________ > devs mailing list > [email protected] > http://lists.xwiki.org/mailman/listinfo/devs -- Thomas Mortagne _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
