Skip to content

Solves issue #7044 refactoring for record, inner to outer and pullup. #8453

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

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

homberghp
Copy link
Contributor

@homberghp homberghp commented Apr 24, 2025

This PR solves issue #7044

The issue describes that record info was lost when doing and inner to outer or pull up refactoring.

In this pull request

  • tests to show the issue.
  • junit test helper to make test output better understandable: AssertLinesEqualHelpers.java
  • added factory for Record classTree in TreeFactory and TreeMaker
  • improved methods in CasualDiff and VeryPretty to pick op parameters from record in constructor, so that
    var args are preserved.

And also: minimal drive-by changes.

This is hopefully my final attempt.

This PR solves issue apache#7044

The issue describes that record info was lost when doing and inner to outer or pull up refactoring.

In this pull request

tests to show the issue.
junit test helper to make test output better understandable: AssertLinesEqualHelpers.java
added RECORD case label where appropriate, for instance where the record template should be used.
added factory for Record classTree in TreeFactory and TreeMaker
added/improved methods in CasualDiff and VeryPretty to pick op parameters from record in constructor, so that
var args are preserved.
@homberghp homberghp changed the title Issue7044d Solves issue #7044 refactoring for record, inner to outer and pullup. Apr 24, 2025
@neilcsmith-net
Copy link
Member

This is the seventh PR opened that claims to solve this issue! Please make sure PRs are ready for review when opened, or mark them as draft so people know how to treat them. Some of us are tracking all notifications in the repo. And please, in most cases force push your original branch to keep the original PR open so the connection to any discussions and review is not lost.

Is this PR reviewable at this point?

 Also added check for arrayness to avoid unnecessary work
@homberghp
Copy link
Contributor Author

This is the seventh PR opened that claims to solve this issue! Please make sure PRs are ready for review when opened, or mark them as draft so people know how to treat them. Some of us are tracking all notifications in the repo. And please, in most cases force push your original branch to keep the original PR open so the connection to any discussions and review is not lost.

Sorry for the inconvenience, if I messed up. According to lahodaj and mbien my earlier attempts were way too big, to which I agree. Rolling back my rejected changes was undoable, so I simply started again and picked the usable parts
of the earlier attempt.

Is this PR reviewable at this point?

Yes, its is reviewable and way smaller.

It passes all tests, including some I had to add after I found some additional issues, where previous attempts messed
up unrelated tests, in particular Rename.
In this version I stick to the bare minimum to in the code, although some more work has been done on the testing front.

CasualDiff and its friends put up a fair fight to understand them.

It was a steep learning curve for me. Thank you for your patience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants