review: fix: Adjust the source position for annotated enum values #4780
Conversation
The source position is not computed correctly when an enum value/constructor call is annotated
There might be annotations on an enum field/constructor call, the source position starts after these enum
gtoison
changed the title
| @@ -420,6 +420,13 @@ SourcePosition buildPositionCtElement(CtElement e, ASTNode node) { | |||
| AllocationExpression allocationExpression = (AllocationExpression) node; | |||
| if (allocationExpression.enumConstant != null) { | |||
| FieldDeclaration fieldDeclaration = allocationExpression.enumConstant; | |||
|
|
|||
| Annotation[] annotations = allocationExpression.enumConstant.annotations; | |||
There was a problem hiding this comment.
There are comments below, starting with 1). I think it would make sense to add one for annotations too and change the numeration accordingly.
There was a problem hiding this comment.
Agreed, I have updated the comments as proposed
| FileUtils.deleteDirectory(OUTPUT_PATH.toFile()); | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
GithubIssue already serves as test annotation, so @Test can be removed.
There was a problem hiding this comment.
OK, I have removed the redundant test annotation
| import org.apache.commons.io.*; | ||
| import org.hamcrest.*; | ||
| import org.junit.*; | ||
| import static org.junit.Assert.*; |
There was a problem hiding this comment.
You should avoid using JUnit 4. The assertThat method should be imported from org.hamcrest.MatcherAssert instead.
There was a problem hiding this comment.
I started from another test in the same package, but yes it's better to write the new unit tests with JUnit 5. I have updated it
| assertThat("Output file for " + path + " should exist", OUTPUT_PATH.resolve(path).toFile().exists(), | ||
| CoreMatchers.equalTo(true)); | ||
|
|
||
| String content = new String(Files.readAllBytes(OUTPUT_PATH.resolve(path)), StandardCharsets.UTF_8); |
There was a problem hiding this comment.
Very minor nitpick: You can use Files.readString(...) here, which also defaults to UTF-8 directly.
|
Thanks for looking into it, if that's OK I'll remove the test in PositionTest.java |
readString() already returns a String
|
Fixes #4332 btw. |
"Cannot compare this:...when trying to print the code with SniperThe fix seems to work but I have very very little experience on the project so this might very well be misguided, there really seem to be a bug though!