Simplify annotation handling in SpringJUnit5Check #157
Conversation
There was a problem hiding this comment.
Thanks for the PR, @davebarda. I've left a few comments. Could you please take a look and, if you agree and have the time to do so, update the proposal accordingly?
...format-checkstyle/src/main/java/io/spring/javaformat/checkstyle/check/SpringJUnit5Check.java
Show resolved
Hide resolved
| @@ -75,14 +67,6 @@ | |||
| BANNED_IMPORTS = Collections.unmodifiableSet(bannedImports); | |||
| } | |||
|
|
|||
| private static void addAnnotation(Set<String> annotations, String annotation) { | |||
There was a problem hiding this comment.
I think that removing this method and the usage of it will introduce a regression. The javadoc for AnnotationUtil.containsAnnotation(DetailAst, List<String>) says that it "accepts both simple and fully-qualified names". If a user has annotated a method with, for example, @org.junit.Test rather than importing org.junit.test and annotating with @Test, this change will cause the usage to be missed.
There was a problem hiding this comment.
This method accepts both simple and fully-qualified names, e.g. "Override" will match both java.lang.Override and Override.
Which means that if we are using only the non FQN then we receive a check for both, there is also a test for that:
JUnit5BadAnnotation
There was a problem hiding this comment.
if we are using only the non FQN
I don't think we can't be certain that this is the case. One user could write @org.junit.Test while another could write @Test along with import org.junit.Test.
There was a problem hiding this comment.
What I meant is that I believe it catches both
There was a problem hiding this comment.
Sorry, I see what you mean now. I agree that just the simple names should be sufficient.
We try to keep issues and pull requests focused on a single change. To that end, would you mind splitting the corrections to JUnit5BadModifier and the simplifications above into separate pull requests?
There was a problem hiding this comment.
Sure, I have updated this one to be the refactor, I'll create a new one for the test
spring-javaformat/spring-javaformat-checkstyle/src/test/resources/source/JUnit5BadModifier.java
Outdated
Show resolved
Hide resolved
| @@ -75,14 +67,6 @@ | |||
| BANNED_IMPORTS = Collections.unmodifiableSet(bannedImports); | |||
| } | |||
|
|
|||
| private static void addAnnotation(Set<String> annotations, String annotation) { | |||
There was a problem hiding this comment.
if we are using only the non FQN
I don't think we can't be certain that this is the case. One user could write @org.junit.Test while another could write @Test along with import org.junit.Test.
…" in order to add it in a different PR This reverts commit 152e98d.
davebarda
changed the title
wilkinsona
changed the title
Checkstyle's AnnotationUtil.containsAnnotation automatically handles simple and fully qualified names. For example, Override matches both java.lang.Override and Override. This means that SpringJUnit5Check need only care about the simple names of the annotations that it wants to detect. See gh-157
|
@davebarda Thank you very much for making your first contribution to Spring Java Format. |
No description provided.