fix(scanner): add missing scan properties based on a new powerful specification #966
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
monperrus
force-pushed
the
test-scanner
branch
from
0449873 to
752f0d0
Compare
tdurieux
reviewed
Nov 12, 2016
| // and then we can have smaller clean tested pull requests to see the impact of the change | ||
| // cp ./target/generated/spoon/reflect/visitor/CtBiScannerDefault.java ./src/main/java/spoon/reflect/visitor/CtBiScannerDefault.java | ||
| //assertThat(build(new File("./src/main/java/spoon/reflect/visitor/CtBiScannerDefault.java")).Class().get(CtBiScannerDefault.class)) | ||
| // .isEqualTo(build(new File("./target/generated/spoon/reflect/visitor/CtBiScannerDefault.java")).Class().get(CtBiScannerDefault.class)); |
There was a problem hiding this comment.
this test is useless without assert.
we don't necessarily want to hard-wired the relation bewteen CtScanner and CtBiScannerDefault.java
Yes we went because CtBiScanner is used for the equality of two elements. Without this assertion we may have bug in equals
tdurieux
reviewed
Nov 12, 2016
| @@ -122,6 +137,7 @@ public void testGenerateCloneVisitor() throws Exception { | |||
| launcher.run(); | |||
|
|
|||
| // cp ./target/generated/spoon/support/visitor/clone/CloneBuilder.java ./src/main/java/spoon/support/visitor/clone/CloneBuilder.java | |||
| // cp ./target/generated/spoon/support/visitor/clone/CloneVisitor.java ./src/main/java/spoon/support/visitor/clone/CloneVisitor.java | |||
There was a problem hiding this comment.
why did you copy the line?
tdurieux
reviewed
Nov 12, 2016
| t2.add(computeSimpleSignature((CtMethod<?>) invoc.getExecutable().getExecutableDeclaration())); | ||
| } | ||
| assertEquals("CtScanner contract violated for "+t.getSimpleName(), t1, t2); | ||
| } |
tdurieux
reviewed
Nov 12, 2016
| final ActualCounterScanner actual = new ActualCounterScanner(); | ||
| actual.biScan(pack, pack.clone()); | ||
| assertEquals(expected.expectedCounter, actual.counter); |
There was a problem hiding this comment.
removes ActualCounterScanner.counter if not used
monperrus
force-pushed
the
test-scanner
branch
from
752f0d0 to
3935a80
Compare
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The idea is no annotate the metamodel methods with Spoon-specific annotations:
@DerivedPropertyfor getters that are derived from other properties@UnsettablePropertyfor setters that are not usable in certains casesThen, the core contract is that
CtScannermust visit all settable, non-derived properties.By doing this, I found a couple of bugs in CScanner (property not visited)