-
-
Notifications
You must be signed in to change notification settings - Fork 352
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
New feature: allow spoon to do static imports for methods and fields #1040
Conversation
…ith an or instead of an and. Now the test is failing, proof that the assertion is checked :)
… a local variable in the same block or by a field (or a variable in another block). First detection implemented but patch not satisfying.
…sing autoimport
…rus/spoon into martin" This reverts commit 2609284, reversing changes made to 49d6e68.
…operties. Some choices have been done concerning some set methods in order to pass the test we should discuss.
Fix conflicts by solving imports.
… set to true or when it is set to false. When set to false, the scanner will detect name conflict and automatically import necessary types.
Solve conflict.
Solve in using version of inria/master
…ble mode. Also improve tests. Related to #1027
…rtScannerImpl. Fix one test but failed lot others. Still working.
…e parent class name if not imported.
} | ||
|
||
@Override | ||
public void computeImports(CtElement element) { | ||
imports.clear(); | ||
classImports.clear(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have to clear the 3 lists
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I push the change.
cool! in Spoon, we aim at maximum backward compatibility and not to change the signature public methods, esp. in interfaces. to this extent, I propose to keep computeImports as is and to introduce a new method (eg computeAllImports). this will greatly reduce the impact in the tests (and in client code also). (fortunately isImported(CtReference ref) is backward compatible) |
@@ -32,7 +32,7 @@ public void testCheckAssignmentContracts() throws Exception { | |||
|
|||
private String buildResourceAndReturnResult(String pathResource, String output) { | |||
Launcher spoon = new Launcher(); | |||
//spoon.setArgs(new String[]{"--with-imports"}); | |||
//spoon.setArgs(new String[]{"--with-classImports"}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrong refactoring?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uh. Yes, I may have done it a bit fast. Apparently I leave some occurences also in comments. I will fix that.
Ok but then DefaultJavaPrettyPrinter will use the computeAllImports, right? |
…e interface ImportScanner for backward compatibility reason as suggested by @monperrus
…atic field. Change test according that. #1027
assertEquals("A.myField", aClass.getElements(new TypeFilter<>(CtFieldWrite.class)).get(0).toString()); | ||
assertEquals("finalField", aClass.getElements(new TypeFilter<>(CtFieldWrite.class)).get(1).toString()); | ||
assertEquals("B.finalField", aClass.getElements(new TypeFilter<>(CtFieldWrite.class)).get(1).toString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check
|
||
assertEquals(2, imports.size()); | ||
Set<String> expectedImports = new HashSet<>( | ||
Arrays.asList("spoon.test.imports.testclasses.internal3.Foo", "java.io.File")); | ||
Set<String> actualImports = imports.stream().map(CtTypeReference::toString).collect(Collectors.toSet()); | ||
Set<String> actualImports = imports.stream().map(CtReference::toString).collect(Collectors.toSet()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Undo change
@@ -51,7 +52,7 @@ public void setup() throws Exception { | |||
public void testQualifiedThisRef() { | |||
DefaultJavaPrettyPrinter printer = new DefaultJavaPrettyPrinter(factory.getEnvironment()); | |||
CtType<?> ctClass = factory.Type().get(QualifiedThisRef.class); | |||
Collection<CtTypeReference<?>> imports = printer.computeImports(ctClass); | |||
Collection<CtReference> imports = printer.computeImports(ctClass); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Undo change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact this one should remain the same as it's the method computeImports from DefaultJavaPrettyPrinter (not defined in any interface) which must return CtReference because it calls computeAllImports from ImportScanner.
assertEqualsFieldAccess(new ExpectedTargetedExpression().type(CtFieldWrite.class).declaringType(expectedType).target(expectedTypeAccess).result("p"), staticElements.get(0)); | ||
|
||
// Changing behaviour when writing static field, it is now writed using the class name | ||
assertEqualsFieldAccess(new ExpectedTargetedExpression().type(CtFieldWrite.class).declaringType(expectedType).target(expectedTypeAccess).result("Foo.p"), staticElements.get(0)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Undo change
Ok to merge this now @monperrus ? |
Improve
ImportScannerImpl
to allow Spoon to do static imports.Change the interface of
ImportScanner
to return list of CtReference when doing computeImports.Lots of tests had to be changed as with autoImports it automatically imports all static elements.