-
Notifications
You must be signed in to change notification settings - Fork 472
Remove fully qualified JavaDoc references from TypesInUse for RemoveUnusedImports
#5738
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
base: main
Are you sure you want to change the base?
Remove fully qualified JavaDoc references from TypesInUse for RemoveUnusedImports
#5738
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
rewrite-java-test/src/test/java/org/openrewrite/java/RemoveUnusedImportsTest.java
Outdated
Show resolved
Hide resolved
@Nested
@Issue("https://github.com/openrewrite/rewrite/pull/5738")
class removeUnusedImportsDoc {
@Test
void usedInDoc() {
rewriteRun(
java(
"""
import java.util.Date;
import java.util.List;
/**
* referencing {@link Date} only in doc
*/
class Test {
List list;
}
"""
)
);
}
@Test
void usedInDocFullQualified() {
rewriteRun(
java(
"""
import java.util.Date;
import java.util.List;
/**
* referencing {@link java.util.Date} only in doc
*/
class Test2 {
List list;
}
""",
"""
import java.util.List;
/**
* referencing {@link java.util.Date} only in doc
*/
class Test2 {
List list;
}
"""
)
);
}
}still the case seem to be fixed, but i came across another issue, not able to fix the impl. Feel free to incorporate. |
TypesInUse for RemoveUnusedImports
rewrite-java-test/src/test/java/org/openrewrite/java/RemoveUnusedImportsTest.java
Show resolved
Hide resolved
rewrite-java-test/src/test/java/org/openrewrite/java/RemoveUnusedImportsTest.java
Show resolved
Hide resolved
| // Fully qualified Javadoc references are _using_ those types as much as they are just references; | ||
| // TypesInUse also determines what imports are retained, and for fully qualified these can be dropped | ||
| return cursor.getValue() instanceof J.FieldAccess && | ||
| cursor.getPathAsStream().anyMatch(o -> o instanceof Javadoc.Reference); |
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.
Since this code is a bit performance critical, I think we may want to optimize this, so that we also have other parent types, which break the cursor walking. Can we perhaps also break on J.Block?
Also, what if we have a reference to a nested type, like Outer.Inner? It seems like the Outer import would still be required.
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.
Good point, thanks! Added a first loop for the J.Block; haven't yet explored the Outer.Inner, but worst case there we would not remove a stray import, which I'd consider to be fairly niche and perhaps not worth handling separately here.
|
no going to finish this, feel free to continue. Thanks. |
| Iterator<Object> path = cursor.getPath(); | ||
| while (path.hasNext()) { |
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.
Capturing a note from Slack: some performance concerns here due to the use of cursor.getPath().
What's changed?
rewritecapability junit-team/junit-framework#4708What's your motivation?
Anything in particular you'd like reviewers to focus on?
Anyone you would like to review specifically?
Have you considered any alternatives or workarounds?
Any additional context
Checklist