-
Notifications
You must be signed in to change notification settings - Fork 7
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
Support sources located in an unnamed package #64
Support sources located in an unnamed package #64
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.
One note.
(W.r.t. the tests: I tried my best to pattern-match and follow the established approach.)
@@ -306,12 +309,15 @@ public void visitClassDef(JCTree.JCClassDecl classDecl) { | |||
if (classDecl.sym != null && classDecl.sym.getNestingKind() == NestingKind.TOP_LEVEL && !recipes.isEmpty()) { | |||
boolean outerClassRequired = descriptor == null; | |||
try { | |||
Symbol.PackageSymbol pkg = classDecl.sym.packge(); |
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.
Deduplicating classDecl.sym.packge()
and classDecl.sym.fullname.toString()
at the top of this method would yield an NPE, unless more things are refactored. But that would likely change the indentation of most of the code in this method, which would make the PR way harder to review. Seems better to refactor that separately (if at all).
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.
Thanks for the fix! Hadn't been taken into account indeed, but glad to see it resolved
Force-pushed to fix the license violation. (Didn't notice the review; sorry!) |
All good! Released v1.4.2 just now. Thanks again! |
See PicnicSupermarket/error-prone-support#775 (comment) and subsequent analysis.
Besides the new unit tests, I also verified that this change resolves the issue in the aforementioned Error Prone Support PR.