Thank you! Now we have 3 +1 and 1 +0.
Vote passed. I will merge the branch. Best Regards, /Andreas 2012-10-31 14:48, Thomas Mortagne skrev: > +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 > > _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
