-
-
Notifications
You must be signed in to change notification settings - Fork 353
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
review: fix: getTypeDeclaration() on nested record type #5015
review: fix: getTypeDeclaration() on nested record type #5015
Conversation
Co-authored-by: Hannes Greule <SirYwell@users.noreply.github.com> Co-authored-by: Martin Wittlinger <wittlinger.martin@gmail.com>
42ae027
to
b4eddca
Compare
@I-Al-Istannen @SirYwell could both of you also mark this PR for approval (after a review) and we simply merge it? |
Github won't let me approve it as I am the author. But consider this my approval :P |
@monperrus could you have a brief look and merge this PR? As all of us three are (co)authors. |
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.
Looks functionally correct, I have a couple of minor comments
src/main/java/spoon/support/reflect/declaration/CtTypeImpl.java
Outdated
Show resolved
Hide resolved
6024979
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 everyone. Now to see if I can manage to merge without removing all coauthorships.
thanks all! |
The nested visitor did not consider records as it was written before their existence.
We might want to find a better upgrade story for such cases.