Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

review feat(role): field annotation with a role #1368

Merged
merged 10 commits into from
Jun 9, 2017

Conversation

tdurieux
Copy link
Collaborator

@tdurieux tdurieux commented Jun 6, 2017

for each field of spoon defines for which interface it is the property

This PR is not finish I would like to have your feedback.

@tdurieux tdurieux changed the title feat(property): spoon property WIP feat(property): spoon property Jun 6, 2017
CtExpression<A> assignment;

@SpoonProperty(clazz = CtTypeReference.class, name = SpoonProperty.PropertyName.TYPE)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CtTypedElement

@pvojtechovsky
Copy link
Collaborator

I did not understood the purpose. What can profit from that?

The mapping of fields to correct property names, might make sense. But may be optional solution would be to rename fields, whose name do not fit to their property name. Then we might have simple rule: field name == property name (I have not checked if it is possible in all cases)

The mapping of property to interface SpoonProperty#clazz is ambiguous.
A) remove it completely (or let me understand what it is good for)
B) use an array of interfaces where this property occurs.

@pvojtechovsky
Copy link
Collaborator

Did I understood well, that this PR is a step on the way to the meta model of spoon model?

In such case I suggest something like this:

C1) we need a property descriptor for each spoon model property. Properties are e.g. CtTypedElement#type or CtNamedElement#name
C2) the property descriptor knows interface(s?) on which this property is defined. Or it has at least method #hasProperty(CtElement target) boolean which returns true if the target element has property defined by this property descriptor.
C3) the property descriptor knows type of the property. E.g. List or 'Map' or 'Set' or single value
C4) for each type from C3, the property descriptor is implemented as sub class/interface of property descritor. So we will have 'list property descriptor', 'map property descriptor', ..., 'single value property descriptor'
C5) each property descriptor defines spoon model accessing methods. For example single value property descriptor would have accessors: #getPropertyValue(CtElement target) and #setPropertyValue(CtElement target, Object value)

I1) These property descriptors might be accessible as attribute of SpoonProperty.PropertyName enum. For example SpoonProperty.PropertyName.DECLARING_TYPE.getDescriptor() would return property descriptor, which might be used to access meta information about this property and to read/modify appropriate field of the spoon model.
I2) We need a CtScannerWithContext, which would provide access to property descriptor of just scanned property
...

@monperrus
Copy link
Collaborator

Did I understood well, that this PR is a step on the way to the meta model of spoon model?

yes

C1) we need a property descriptor for each spoon model property. Properties are e.g. CtTypedElement#type or CtNamedElement#name
C3) the property descriptor knows type of the property. E.g. List or 'Map' or 'Set' or single value

Yes, this will be implemented as additional information in the to-be-renamed SpoonProperty.PropertyName enumeration (=I1)

C2) the property descriptor knows interface(s?) on which this property is defined.

Yes, this is the final goal

Or it has at least method #hasProperty(CtElement target) boolean which returns true if the target element has property defined by this property descriptor.

Runtime support is welcome in subsequent PRs

C5) each property descriptor defines spoon model accessing methods. For example single value property descriptor would have accessors: #getPropertyValue(CtElement target) and #setPropertyValue(CtElement target, Object value)

Yes, we aim at this too.

I2) We need a CtScannerWithContext, which would provide access to property descriptor of just scanned property

Welcome in subsequent PR.

It seems we're on the same wavelength.

@monperrus
Copy link
Collaborator

monperrus commented Jun 7, 2017

per our discussion:

@SpoonProperty -> @MetamodelPropertyField
SpoonProperty.PropertyName. -> reuse and rename CtPathRole -> CtRole
name = SpoonProperty.PropertyName.LEFT_OPERAND) -> role = CtRole.LEFT_OPERAND) 

contract: all non-final fields must be annotated with @MetamodelPropertyField (checkable with architecture test)

todo: remove the clazz information, create @PropertyGetter(role = X), @PropertySetter(role = X), in another PR that will be merged first (for this PR: contract 1): all roles are declared at least once in a metamodel interface method) and maybe another PR for contract 2): all methods of metamodel interfaces are annotated with either @Unsettable, @DerivedProperty @PropertyGetter or @PropertySetter

@tdurieux
Copy link
Collaborator Author

tdurieux commented Jun 9, 2017

I updated this PR but I cannot create the contract because of these fields:

private CtTypeReference<Void> voidType;
transient Factory factory;
protected CtElement parent;
SourcePosition position = SourcePosition.NOPOSITION;
Map<String, Object> metadata;
private CtMethod<T> valueOfMethod;

@tdurieux tdurieux changed the title WIP feat(property): spoon property WIP feat(role): field annotation with a role Jun 9, 2017
@tdurieux
Copy link
Collaborator Author

tdurieux commented Jun 9, 2017

@monperrus Done for me


@Test
/**
* contract: all all non-final fields must be annotated with {@link spoon.reflect.annotations.MetamodelPropertyField}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix comment

@surli
Copy link
Collaborator

surli commented Jun 9, 2017

It should be great to have a test to check if there is a getter/setter in one interface of the class for all field with a MetamodelPopertyRole.

@tdurieux tdurieux force-pushed the feat_spoon_property branch from 0b417bb to cb915a5 Compare June 9, 2017 14:39
@tdurieux
Copy link
Collaborator Author

tdurieux commented Jun 9, 2017

Done

@surli surli changed the title WIP feat(role): field annotation with a role review feat(role): field annotation with a role Jun 9, 2017
@INRIA INRIA deleted a comment from spoon-bot Jun 9, 2017
@tdurieux tdurieux force-pushed the feat_spoon_property branch from cb915a5 to f25e3b2 Compare June 9, 2017 15:02
@INRIA INRIA deleted a comment from spoon-bot Jun 9, 2017
@surli surli merged commit e738a1b into INRIA:master Jun 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants