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

fix: fix wrong import on annotated parameter #3320 #3341

Merged
merged 5 commits into from
Apr 24, 2020

Conversation

gibahjoe
Copy link
Contributor

@gibahjoe gibahjoe commented Apr 21, 2020

fix #3320

Copy link
Collaborator

@monperrus monperrus left a comment

Choose a reason for hiding this comment

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

Nice!

Congrats on the first contribution! That's really cool.

See comments in PR.

--Martin

pom.xml Outdated
@@ -159,6 +159,14 @@
<artifactId>maven-invoker</artifactId>
<version>3.0.1</version>
</dependency>
<!-- https://mvnrepository.com/artifact/javax.validation/validation-api -->
Copy link
Collaborator

Choose a reason for hiding this comment

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

The test is in noclasspath mode. Remove this or work in fullclasspath?

@@ -1777,4 +1778,22 @@ public void testImportByJavaDoc() throws Exception {
}
}

@Test
public void testThatCorrectImportsAreGeneratedForJavaxAnnotatedElements() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

In Spoon, the tests come with a one-line contract describing the test intention, something like

// contract: annotations are not added on import statements

could you add the contract that is tested? Thanks!

CompilationUnit compilationUnit = l.getFactory().CompilationUnit().getOrCreate(objectCtType);

for (CtImport anImport : compilationUnit.getImports()) {
System.out.println(anImport.prettyprint());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove System.out from tests.

@gibahjoe
Copy link
Contributor Author

Thanks.I have attended to the pr comments

Copy link
Collaborator

@MartinWitt MartinWitt left a comment

Choose a reason for hiding this comment

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

See comments, rest looks good. Awesome work :)

Comment on lines 7 to 10
* @author Gibah Joseph
* Email: [email protected]
* Apr, 2020
**/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Personally i hate reading this. It feels like useless information, which get outdated easily. Git can provide this information better. Maybe remove it?

Comment on lines 4 to 7
* @author Gibah Joseph
* Email: [email protected]
* Apr, 2020
**/
Copy link
Collaborator

Choose a reason for hiding this comment

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

see other comment

Comment on lines 9 to 13
/**
* @author Gibah Joseph
* email: [email protected]
* Apr, 2020
**/
Copy link
Collaborator

Choose a reason for hiding this comment

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

see other comment

Comment on lines 1784 to 1791
final Launcher l = new Launcher();
Environment e = l.getEnvironment();

e.setNoClasspath(true);
e.setAutoImports(true);
l.addInputResource("src/test/java/spoon/test/imports/testclasses/badimportissue3320/source/TestSource.java");
l.run();

Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe a pit "picky", but short names like e and l makes reading hard.

CtType<TestSource> objectCtType = l.getFactory().Type().get(TestSource.class);
CompilationUnit compilationUnit = l.getFactory().CompilationUnit().getOrCreate(objectCtType);

assertEquals(1, compilationUnit.getImports().stream().filter(ctImport -> ctImport.prettyprint().equals("import spoon.test.imports.testclasses.badimportissue3320.source.other.SomeObjectDto;")).count());
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe a linebreak, but not mandatory

@MartinWitt
Copy link
Collaborator

LGTM, great work

@monperrus monperrus changed the title fix: annotated parameter wrong import #3320 fix: fix wrong import on annotated parameter #3320 Apr 24, 2020
@monperrus monperrus merged commit 02beef2 into INRIA:master Apr 24, 2020
@monperrus
Copy link
Collaborator

Thanks a lot and congrats on your first contribution!

@gibahjoe
Copy link
Contributor Author

Thanks

I-Al-Istannen added a commit to I-Al-Istannen/spoon that referenced this pull request Jul 5, 2023
I-Al-Istannen added a commit to I-Al-Istannen/spoon that referenced this pull request Jul 5, 2023
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.

Wrong javax import statement causing compilation error
3 participants