Fix package name and app name replacement - #376
Conversation
kelset
left a comment
There was a problem hiding this comment.
Thanks for this PR — and sorry it took us such a long time to come back around to it.
I took another pass through it as part of a maintenance review of the open PRs, and the underlying bug still looks real from what I can see: path replacement and content replacement are not fully in sync yet in some Android diffs, so I do think this is still pointing at a valid issue.
That said, I don’t think we can merge this branch as-is anymore because it’s old and the surrounding code has changed since then. In particular, this part of the diff rendering flow has had other fixes land since this PR was opened, including #378, so I’d want to make sure we don’t accidentally reintroduce any of those older rendering problems.
If you’re still interested in updating this PR, could you please:
- rebase/update the branch on current
master - verify the fix still works against the current
Diff.tsximplementation - make sure it doesn’t regress the duplicated-hunks/rendering issue that was fixed later in #378
- add/update a regression test if practical
If you’re up for refreshing it, I’d be happy to take another look after that. And if not, no worries at all — we can always revisit with a fresh PR from current master later.
Summary
the app name and package name were not being properly changed inside the diff files, only in the diff path, this is happening because the tokenizer is using the old
hunksinstead ofupdatedHunks. I also addedgetHunksWithAppNameas a dependency to make sure the files are automatically updated when the app name or package name are changed. This isn't introducing any new features, simply fixing what appears to be a bug.Test Plan
Before:
After:
What are the steps to reproduce?
Checklist
README.md(if needed)