Community
Participate
Working Groups
As discussed in bug #282957, the Callback interface needs to be extended to deal with incremental generation. The proposed changes are: * The pre() method should return a boolean value. If this returns false, processing of the AST should skip this element. All callers have to evaluate this return value. * The post() method should receive the SyntaxElement and the ExecutionContext as additional parameters. Only then can a callback reasonably infer which element has just been processed (other than maintaining its own stack). * The method initialize(WorkflowContext workflowContext, Issues issues) should be added to the interface to allow the Callback access to slots defined in the workflow. As mentioned in bug #282957, these are breaking API changes. Xpand is 0.7, so this should be allowed. There are not many users of this interface. One additional note. Declaring the initialize() method now binds the Callback to MWE. Since the Callback is technically based at the Xtend level, I don't really like the fact that Xtend is now bound to MWE. Maybe we should consider a specialized WorkflowAwareCallback interface that extends the basic Callback interface and adds the initialize() method.
Created attachment 141650 [details] Proposed patch This patch implements the proposed changes. The call to initialize() is done in AbstractExpressionsUsingWorkflowComponent. This may or may not be the best place...
(In reply to comment #0) > * The pre() method should return a boolean value. If this returns false, > processing of the AST should skip this element. All callers have to evaluate > this return value. I think we should try to come up with a better name for "pre" as it gets kind of a veto-semantic. > * The post() method should receive the SyntaxElement and the ExecutionContext > as additional parameters. Only then can a callback reasonably infer which > element has just been processed (other than maintaining its own stack). > * The method initialize(WorkflowContext workflowContext, Issues issues) should > be added to the interface to allow the Callback access to slots defined in the > workflow. > > As mentioned in bug #282957, these are breaking API changes. Xpand is 0.7, so > this should be allowed. There are not many users of this interface. > Even if we are in 0.7 we should try to come up with a nonbreaking solution. What about and interface "CallbackExtension" with overloads the post and pre methods. The semantic would be: If a workflow component uses the extension interface, the old pre- and post-methods will not be invoked. Legacy components call the simple pre- and post. > One additional note. Declaring the initialize() method now binds the Callback > to MWE. Since the Callback is technically based at the Xtend level, I don't > really like the fact that Xtend is now bound to MWE. Maybe we should consider a > specialized WorkflowAwareCallback interface that extends the basic Callback > interface and adds the initialize() method. > Good point. The new method initialize would break existing clients and introduce an unwanted dependency. I'ld prefer "InitializableCallback" instead.
This CallbackExtension interface's semantics would be entirely different from the old Callback semantics. If a client implements the old interface, we call the old pre() and post() methods. If a client implements the new interface, we only call the new pre() (with veto return value) and post() (with additional parameters) methods. The client still has to implement the old pre() and post() methods, although they are definitely never called. Not nice for the client. To improve this, we could create an entirely new VetoableCallback interface that does _not_ extend the old Callback interface. We'll remain perfectly backwards compatible. But Xpand and Xtend will then have to deal with two kinds of Callbacks, i.e. setCallback(Callback) and setCallback(VetoableCallback). Not nice, too. OR, we'll introduce a third (marker) interface BaseCallback. setCallback() can be rewritten to accept a BaseCallback instead of a Callback. This change is more or less backwards-compatible. This will lead us to: interface BaseCallback { } interface Callback extends BaseCallback { // regular old callback signature } interface VetoableCallback extends BaseCallback { // new call back signature, but without initialize() } interface WorkflowAwareCallback extends BaseCallback { void initialize(WorkflowContext ctx); } => Old callback implementations remain unchanged. The IncrementalGenerationCallback will implement both VetoableCallback and WorkflowAwareCallback.
(In reply to comment #3) > This CallbackExtension interface's semantics would be entirely different from > the old Callback semantics. If a client implements the old interface, we call > the old pre() and post() methods. If a client implements the new interface, we > only call the new pre() (with veto return value) and post() (with additional > parameters) methods. The client still has to implement the old pre() and post() > methods, although they are definitely never called. Not nice for the client. > The old ones will be called by old workflow components that are implemented from scratch. Clients can implement pre() and post() as no-ops if they cannot do something meaningful without the additional information. > To improve this, we could create an entirely new VetoableCallback interface > that does _not_ extend the old Callback interface. We'll remain perfectly > backwards compatible. But Xpand and Xtend will then have to deal with two kinds > of Callbacks, i.e. setCallback(Callback) and setCallback(VetoableCallback). Not > nice, too. > > OR, we'll introduce a third (marker) interface BaseCallback. setCallback() can > be rewritten to accept a BaseCallback instead of a Callback. This change is > more or less backwards-compatible. This will lead us to: > > interface BaseCallback { > } > > interface Callback extends BaseCallback { > // regular old callback signature > } > > interface VetoableCallback extends BaseCallback { > // new call back signature, but without initialize() > } > > interface WorkflowAwareCallback extends BaseCallback { > void initialize(WorkflowContext ctx); > } > > => Old callback implementations remain unchanged. The > IncrementalGenerationCallback will implement both VetoableCallback and > WorkflowAwareCallback. > Sounds reasonable to me. However I'm not that into backwards compatibility and if nobody thinks that changing the API of the Callback interface would be a big deal .. From my point of view it should be pretty straight forward to migrate existing implementations and therefore it is not a problem from my point of view. I just want to ensure that the migration path will be documented somewhere.
We should deprecate the old callback interface and provide a thin adapter to the new one. This would encapsulate the legacy issue in that adapter, Xtend would only work on the new interface. I also don't like the MWE reference in the initialize() method. We could as you already proposed either have another interface (e.g. IWorkflowAware). That one shouldn't extend VetoableCallback, because IMHO it's an orthogonal aspect to be initialized with the workflow information. You could also solve it by writing the workflow like so: <workflow> <component class="my.WorkflowAwareCallbackImpl" id="callback"/> <component class="xpand.Generator"> <callback idRef="callback"/> .... </component> </workflow> This would make require a bit more configuration, but wouldn't require us to introduce another concept. I prefer the second solution. :-) I guess that you could even avoid passing information through the WorkflowContext, by just using the same component for reading in the tracing infromation, etc. and doing the callback work. So in the end it would be less configuration for the users.
We clarified final points in a Skype call. Summary: * Callback will be deprecated. * VetoableCallback will be introduced. * The two callbacks will _not_ be related in any sort of inheritance hierarchy. * Generator component will have setters for both Callback and VetoableCallback. * Old Callback will automatically be wrapped into a VetoableCallback. * Internally, we will only work with VetoableCallback. * There will be no "WorkflowAware" callback. To achieve this goal, one can simply implement WorkflowComponent. * The IncrementalGeneration stuff will be rewritten to that end. This will probably make it even easier for consumers to use it in their own workflows.
(Hopefully) final note: The proposed changes (as of comment #6) will _not_ be strictly API compatible, since the ExecutionContext now exposes a VetoableCallback instead of a (simple, old) Callback. The impact to users, however, should be minimal, as we still accept an old Callback upon creation of an ExecutionContext or Generator component.
(In reply to comment #7) > (Hopefully) final note: The proposed changes (as of comment #6) will _not_ be > strictly API compatible, since the ExecutionContext now exposes a > VetoableCallback instead of a (simple, old) Callback. The impact to users, > however, should be minimal, as we still accept an old Callback upon creation of > an ExecutionContext or Generator component. > Agreed.
Changes committed to HEAD.
Bug resolved before Xpand 1.2 release date => Closing
