Skip to main content
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [List Home]
Re: [handly-dev] On the way to non-breaking evolution
Hi!

Yes, I only looked at the current diffs, not the rest of the code, that would have cleared things up.

I still don't feel very comfortable with putting the burden of ensuring unique names for properties on the user - in theory, different plugins from different developers might create properties with the same name and use them on the same model. Maybe that won't be a problem in practice, but you have to decide if it's worth doing something about it. The big problem that I see is that the resulting code would be difficult to debug, with things happening differently depending on which other plugins are installed.

best regards,
Vlad


On Tue, Aug 30, 2016 at 4:33 PM, Vladimir Piskarev <pisv@xxxxx> wrote:
Hi Vlad,
 
Thank you very much for taking a look, and the prompt feedback!
 
These are all very valid concerns, but I hope that they have been addressed, although it might not be obvious in the diffs.
 
* "The properties that are meant to be used as API should be documented" - Absolutely! It becomes even more important to thoroughly document APIs in this more loosely-coupled, context-based design. I tried to document all of the API elements, including the properties. In particular, CREATE_BUFFER is documented in both ISourceFileImpl#hBuffer and Elements#getBuffer besides the JavaDoc for the property itself.
 
* " An alternative could be to define sets of properties that belong together as enums" - Please see org.eclipse.handly.util.ToStringOptions in the master branch :-)
 
* "What about properties' names - is it ok if they aren't unique?" - Please see the JavaDoc for IContext (in the master branch). It specifies that "Context implementations may use an identity-based lookup, name-based lookup, or anything in-between. For portability, keys should be unique instances with unique names." So yes, property names should be unique in a context if portability between different context implementations is to be ensured. Currently, we provide context implementations with identity-based lookup (Context, Contexts) and "name+type" property lookup (GuiceContext, not used currently).
 
Again, thanks for the constructive feedback! I really appreciate it. Please let me know if you see any other issues.
 
Best regards,
Vladimir
 
 
Hi!

Looks nice, but there is one thing that I think is missing (or I missed it in the diffs): the properties that are meant to be used as API should be documented. For example, SourceFile#hBuffer's docs should mention that CREATE_BUFFER is a relevant property for it. 

Also, I am not sure if using the same mechanism for utilities is working well. For example, hToString(Context) -- the properties for this utility are probably going to be unique to it, and my feeling is that a ToStringConfiguration class would be easier to understand. If these values need to be used in the context of a Context (!), then the whole class can be stored in a property instead of each individual value. It's possibly a matter of taste. An alternative could be to define sets of properties that belong together as enums.

What about properties' names - is it ok if they aren't unique? As it is now, one can inadvertently create a property with the same name as another, but with different semantics. I can't check the implementation right now, so I'm doing the lazy thing here :-)

best regards,
Vlad


On Tue, Aug 30, 2016 at 11:30 AM, Vladimir Piskarev <pisv@xxxxx> wrote:
Greetings handly-dev,
 
A major feature of the 0.6 release is the introduction of contexts [1]
as a means of facilitating future evolution of Handly.
 
Fundamentally, a context supplies values (data objects or services)
associated with keys. When used as a method parameter or return value,
it enables a more loosely coupled functional interaction and therefore
allows for more independent variability of the participants of the
interaction (a caller and a callee).
 
Composite contexts can emulate a 'dynamic scope' or 'dynamic binding'
available in some programming languages and used to a great effect in such
extensible systems as EMACS [2].
 
The interface org.eclipse.handly.context.IContext enables such
context-facilitated functional interactions to remain type-safe
by supporting two kinds of keys: org.eclipse.handly.util.Property
and java.lang.Class. The Property class has been moved from the
org.eclipse.handly.model package [3] and generalized to retain
type information at runtime [4] and to be able to provide a default value [5].
 
As a first application of contexts in Handly, the org.eclipse.handly.model.ToStringStyle
class has been replaced with a context [6]. I quite liked the result, which I think is
a more general and evolvable design, so I decided to try and extrapolate it
right to the core by making some of the other methods in the Element hierarchy
context-aware [7].
 
Again, a goal here is to enable non-breaking evolution in the future.
However, such re-design does come with an immediate cost:
 
* It introduces a bit of additional complexity, as is typical with a more loosely
coupled designs (see, for example, [8])
 
* It requires a number of significant breaking changes in existing model
implementations.
 
My judgement is that the complexity is manageable and the cost is justified,
but adopters may think otherwise.
 
That's why I'd like to draw your attention to the patch in Gerrit
 
 
before I merge it.
 
This is a rather large patch, but the most relevent changes are in
the classes Element and SourceFile.
 
Specifically, Element#hBuildStructure signature has changed to take a context
instead of the 'body' and 'newElements' arguments. Implementations are supposed
to create and initialize the necessary bodies and put them in the NEW_ELEMENTS
map in the given context. Besides making for a very simple and generic method
signature, such design makes it very easy to introduce additional parameters
(i.e. new context properties) and allows to deprecate and replace existing ones
(e.g. introduce an element requestor in place of the NEW_ELEMENTS map,
if deemed necessary) in a non-breaking fashion.
 
Similarly, the abstract method SourceFile#hBuildStructure takes an AST
(represented generically as an Object) and a context. The AST is created
with #hCreateAst method that takes a source string and a context, which may
contain parser options. The reconciling infrastructure ensures that a context
passed to the #hReconcile method is propagated (in an augmented form) to
#hCreateAst and, through ReconcileOperation#reconcile, to #hBuildStructure.
In particular, this makes it possible to specify parser options in the #hReconcile
method's context (see org.eclipse.handly.internal.examples.javamodel.CompilationUnit
#reconcile for an example).
 
There are other changes as well, but those listed above seem to be the most
affecting ones. It may be instructive to take a look at the patch and see how
these changes affect the example model implementations.
 
I would appreciate any feedback. Please let me know if there are
any questions or concerns.
 
Thanks,
Vladimir
 

_______________________________________________
handly-dev mailing list
handly-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://dev.eclipse.org/mailman/listinfo/handly-dev


_______________________________________________
handly-dev mailing list
handly-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://dev.eclipse.org/mailman/listinfo/handly-dev

Back to the top