Review: refactor: move SpoonMetaModel from src/test to src/main by pvojtechovsky · Pull Request #2016 · INRIA/spoon

pvojtechovsky · 2018-05-30T18:52:20Z

The metamodel in runtime is needed by Pattern matcher.

@monperrus I have some questions:

should we somehow merge SpoonMetaModel with spoon.Metamodel ?
where to put all other necessary metamodel classes which are actually in spoon.metamodel ?
the creation of SpoonMetaModel in runtime needs about 3s. I guess we should lazily create it when somebody needs it and cache it. Where? FactoryImpl#metamodel? Or static variable of spoon.Metamodel or ?

monperrus · 2018-05-30T20:27:33Z

should we somehow merge SpoonMetaModel with spoon.Metamodel ?

Yes.

where to put all other necessary metamodel classes which are actually in spoon.metamodel ?

I'm not sure to understand. Which ones?

the creation of SpoonMetaModel in runtime needs about 3s. I guess we should lazily create it when somebody needs it and cache it. Where? FactoryImpl#metamodel? Or static variable of spoon.Metamodel or ?

A static singleton seems fine to me.

pvojtechovsky · 2018-06-01T19:53:19Z

@monperrus I need your feedback concerning design. There are several ways how to do it
A) to move typemembers of SpoonMetaModel into Metamodel
the problem is that constructor of MetamodelConcept is package protected. So we have to either make it public or move MetamodelConcept into package spoon
B) to keep SpoonMetaModel as internal class in package spoon.metamodel and just create it and implement delegate methods from Metamodel ... but how to make it internal and keep it accessible for other package?
C) to move Metamodel from spoon to spoon.metamodel
D) to move MetamodelConcept, etc. from spoon.metamodel to spoon
E) ?

pvojtechovsky · 2018-06-03T18:20:46Z

ping @monperrus

pvojtechovsky · 2018-06-03T18:23:04Z

I suggest to move spoon.Metamodel to spoon.metamodel.Metamodel. WDYT?

monperrus · 2018-06-04T18:49:45Z

I suggest to move spoon.Metamodel to spoon.metamodel.Metamodel. WDYT?

OK for me.

monperrus · 2018-06-05T19:45:50Z

Just passed over the PR and pushed my changes (mostly documentation, several renamings, some refactorings).

This is super meta, I like it!

Two questions:

getRootSuperField: I don't understand at all this method, could you explain more in the Javadoc?
getRelatedMethods (this was before getOwnMethods): is that correct? is that a good name?

pvojtechovsky · 2018-06-05T20:13:20Z

This is super meta, I like it!

thank you. Me too. It opens new ways how to implement new powerful features in Spoon.

getRelatedMethods/getOwnMethods

For example:
CtClass has no OWN methods which provides access to CtRole#NAME, therefore getRelatedMethods will return empty collection when it is called on CtClass and CtRole#NAME. But if the same method is called on CtNamedElement and CtRole#NAME, then it will return getSimpleName, setSimpleName

So I would prefer getOwnMethods or getDeclaredMethods. Because getRelatedMethods sounds more like (all) related method, not only these which are delcared directly in current concept and are not inherited from super type

monperrus · 2018-06-06T07:11:32Z

So I would prefer getOwnMethods or getDeclaredMethods

Just renamed it to getDeclaredMethods. I'm still not sure it should be in this class.

Would mmMethod.getDeclaredMethods() be conceptually equivalent to mmMethod.getConcept().getRelatedMethods(mmMethod)?

pvojtechovsky · 2018-06-06T08:41:38Z

Would mmMethod.getDeclaredMethods() be conceptually equivalent to mmMethod.getConcept().getRelatedMethods(mmMethod)?

yes, it would be conceptually equivalent. So API can be changed like this, but the data should be stored same - so we assure that it behaves same

monperrus · 2018-06-06T15:49:54Z

By working on getDeclaredMethods, I finally got a solution where all tests pass with method getDeclaredMethods / field ownMethods.

Is there something I don't understand? Is there a missing test?

pvojtechovsky · 2018-06-06T17:43:59Z

Is there something I don't understand?

Probably yes. I don't understand your changes - it cannot work. You changed the meaning of members - there is different values now so it fails.

Is there a missing test?

sure. I did not developed that using "Test driven development" rules, so there was a code needed by other tests, which was not directly tested by MetamodelTest.

I will need some time to remember the purpose of own methods, but they are definitely need.

I suggest to rollback the changes related to own methods

pvojtechovsky · 2018-06-06T17:54:54Z

I suggest to rollback your last commit. We can then discuss the state before that commit and I can explain your questions. But your latest changes are too deep and changed a lot.

pvojtechovsky · 2018-06-06T18:19:35Z

Is there a missing test?

CtScannerTest#testScannerCallsAllProperties is not only a test of scanner but also a test of metamodel. I think you agree that CtScanner contains some unique information about spoon model. So "comparing" the CtScanner and Metamodel types and properties is the only test which can check whether all concepts and properties are as expected.

monperrus · 2018-06-06T19:45:10Z

You're right. I'll rollback.

monperrus · 2018-06-07T16:10:43Z

Worked on it. The goal was to:

remove field MMMethod#superTypes in which was redundant with ownMethods
remove a discrepancy: ownMethods also contains other methods from upper in the class put by addSuperField

This has worked well, but for CtScannerTest and ReplaceParametrizedTest.

For handling this, this was the opportunity to simplify two things:

simplify isDerived/isUnsettable
only consider the interface classes in the metamodel, and not the implementation classes

Now all tests pass!

OK with this?

pvojtechovsky · 2018-06-07T19:26:36Z

src/test/java/spoon/reflect/visitor/CtScannerTest.java

 				}

 				MMMethod getter = mmField.getMethod(MMMethodKind.GET);
+				System.err.println("---"+getter.getSignature());


is it intentional?

pvojtechovsky · 2018-06-07T19:27:51Z

src/test/java/spoon/reflect/visitor/CtScannerTest.java


-				if(getter.getName().equals("getComments") && leafConcept.getModelInterface().isSubtypeOf(ctRefRef)) {
+				if(getter.getName().equals("getComments") && leafConcept.getMetamodelInterface().isSubtypeOf(ctRefRef)
+						) {


is the new line before that line intentional?

pvojtechovsky · 2018-06-07T19:29:34Z

src/test/java/spoon/reflect/visitor/CtScannerTest.java

 			// contract: CtScanner only calls methods that have a role and the associated getter
 			if (calledMethods.size() > 0) {
-				problems.add("CtScanner " + visitMethod.getPosition() + " calls unexpected methods: "+calledMethods);
+				fail("CtScanner " + visitMethod.getPosition() + " calls unexpected methods: "+calledMethods);


why that? A) change all to fail or keep all as problems.add(). WDYT?

pvojtechovsky · 2018-06-07T19:31:20Z

src/test/java/spoon/test/api/MetamodelTest.java

 			mmConcept.getRoleToProperty().forEach((role, mmField) -> {
-				if (mmField.isUnsettable()) {
-					//contract: all unsettable fields are derived too
-					assertTrue("Unsettable field " + mmField + " must be derived too", mmField.isDerived());


Why did you removed that contract? Is it no more true?

it was hard to understand and debug.

now the contract is much easier: if the method or one of its ancestor contains the Derived annotation (resp Unsettable) it is derived (resp unsettable)

the test has been adapted accordingly

These lines was the test you required here #1904
I guess the contract "unsettable implies derived" should be still assert by a test. So I suggest to keep these lines

pvojtechovsky · 2018-06-07T19:32:25Z

src/test/java/spoon/test/replace/ReplaceParametrizedTest.java


 			RoleHandler rh = RoleHandlerHelper.getRoleHandler(o.getClass(), mmField.getRole());
+			if (mmField.getName().equals("interface")) {
+				System.out.println("");


Is it intentional?

forgot the breakpoint, sorry, removed!

pvojtechovsky · 2018-06-07T19:36:57Z

OK with this?

Yes, I like these simplifications. Thank You Martin 👍

monperrus · 2018-06-08T13:09:10Z

Then, I'll merge after Travis, and you can proceed with rebasing the Pattern branch afterwards.

pvojtechovsky · 2018-06-08T16:38:26Z

The Travis failed on timeout (downloading some dependencies), so I rebased to trigger next build

pvojtechovsky · 2018-06-08T17:38:19Z

@surli the travis failed on timeout second time. Any idea?

spoon-bot · 2018-06-09T06:20:38Z

API changes: 1 (Detected by Revapi)

Old API: fr.inria.gforge.spoon:spoon-core:jar:6.3.0-20180608.225255-128 / New API: fr.inria.gforge.spoon:spoon-core:jar:6.3.0-SNAPSHOT

	Class was removed.
Old	class Metamodel
New	none
Breaking	binary: breaking

pvojtechovsky · 2018-06-09T06:57:23Z

@monperrus CI finally passed. It is ready for merge

monperrus mentioned this pull request Jun 1, 2018

review: feature: introduce the concept of "Pattern" #1686

Merged

pvojtechovsky changed the title ~~refactor: move SpoonMetaModel from src/test to src/main~~ HELP refactor: move SpoonMetaModel from src/test to src/main Jun 1, 2018

pvojtechovsky force-pushed the refMoveMetamodel branch from 3558168 to ce0ccf9 Compare June 3, 2018 19:44

pvojtechovsky changed the title ~~HELP refactor: move SpoonMetaModel from src/test to src/main~~ WiP: refactor: move SpoonMetaModel from src/test to src/main Jun 3, 2018

pvojtechovsky changed the title ~~WiP: refactor: move SpoonMetaModel from src/test to src/main~~ Review: refactor: move SpoonMetaModel from src/test to src/main Jun 4, 2018

monperrus force-pushed the refMoveMetamodel branch from ac3c98c to 5cb8998 Compare June 6, 2018 22:15

pvojtechovsky commented Jun 7, 2018

View reviewed changes

pvojtechovsky added 2 commits June 8, 2018 18:35

move SpoonMetaModel from test to main (runtime)

1a38c5d

move Metamodel to spoon.metamodel and merge it with SpoonMetamodel

6bbe16a

pvojtechovsky and others added 15 commits June 8, 2018 18:35

fix tests related to added package

1ca02e1

testShadowModelEqualsNormalModel must still use sources based metamodel

f93a572

fix javadoc

491376b

cleaning

bb996e0

up

b2eb2b2

up

45fcebe

up

4abccf5

added javadoc

d2f1684

up

f99529a

up

e944e89

simplification-derived

8f54193

up

35ca171

up

c34a9e6

up

79f8053

up

48fe277

pvojtechovsky force-pushed the refMoveMetamodel branch from 3687f91 to 48fe277 Compare June 8, 2018 16:37

trigger CI

600fec0

monperrus merged commit 1e6e4a4 into INRIA:master Jun 9, 2018

pvojtechovsky deleted the refMoveMetamodel branch June 9, 2018 08:13

pvojtechovsky mentioned this pull request Jun 10, 2018

Prepare meta-metamodel for migration to src/main #1888

Closed

3 tasks

surli mentioned this pull request Jun 25, 2018

Spoon 7.0.0 is released! #2041

Closed

Uh oh!

Conversation

pvojtechovsky commented May 30, 2018

Uh oh!

monperrus commented May 30, 2018

Uh oh!

pvojtechovsky commented Jun 1, 2018

Uh oh!

pvojtechovsky commented Jun 3, 2018

Uh oh!

pvojtechovsky commented Jun 3, 2018

Uh oh!

monperrus commented Jun 4, 2018

Uh oh!

monperrus commented Jun 5, 2018

Uh oh!

pvojtechovsky commented Jun 5, 2018

Uh oh!

monperrus commented Jun 6, 2018

Uh oh!

pvojtechovsky commented Jun 6, 2018

Uh oh!

monperrus commented Jun 6, 2018

Uh oh!

pvojtechovsky commented Jun 6, 2018

Uh oh!

pvojtechovsky commented Jun 6, 2018

Uh oh!

pvojtechovsky commented Jun 6, 2018

Uh oh!

monperrus commented Jun 6, 2018

Uh oh!

monperrus commented Jun 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pvojtechovsky commented Jun 7, 2018

Uh oh!

monperrus commented Jun 8, 2018

Uh oh!

pvojtechovsky commented Jun 8, 2018

Uh oh!

pvojtechovsky commented Jun 8, 2018

Uh oh!

spoon-bot commented Jun 9, 2018

Uh oh!

pvojtechovsky commented Jun 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants