-
Notifications
You must be signed in to change notification settings - Fork 121
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 handling deeply nested java classes. #423
Conversation
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.
LGTM
|
||
def loadEnclosingClass(clazz: Class[_]): Option[String] = { | ||
binaryToSourceName(clazz) match { | ||
case None if loadedClass.getEnclosingClass != null => |
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.
I think you mean clazz.getEnclosingClass != null
here.
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.
How was the test case even passing with this error? This would mean that more than one level of nesting would not be handled.
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.
Tests passes since if loadedClass
is nested class (regardless how deep) it will be always null.
The only problem with this is that in case that for some reason we don't have source mapping for top enclosing class we will get NullPointerException here. But anyway this was stupid mistake.
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.
LGTM. Thanks @romanowski. Waiting for your feedback.
import sbt.internal.inc.Analysis | ||
import sbt.io.IO | ||
|
||
/** |
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.
Can we remove this header? We don't use them anywhere in the project.
case analysis: Analysis => | ||
analysis.relations.libraryDep._2s | ||
.filter(_.toPath.startsWith(tempDir.toPath)) shouldBe 'empty | ||
|
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.
Remove empty space?
|
||
def loadEnclosingClass(clazz: Class[_]): Option[String] = { | ||
binaryToSourceName(clazz) match { | ||
case None if loadedClass.getEnclosingClass != null => |
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.
How was the test case even passing with this error? This would mean that more than one level of nesting would not be handled.
85bd8a9
to
2aa65c5
Compare
Without fix it reports dependencies to jar/classes produced by same project.
2aa65c5
to
e792676
Compare
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.
LGTM @romanowski, thanks for this work.
Without fix it reports dependencies to jar/classes produced by same project.