Skip to content
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

add mergeText plugin #1908

Closed
wants to merge 23 commits into from
Closed

add mergeText plugin #1908

wants to merge 23 commits into from

Conversation

johnkenny54
Copy link
Contributor

Consolidate adjacent <text> elements and <text> elements with a single <tspan> child.

Resolves #964, resolves #1907

@johnkenny54
Copy link
Contributor Author

There are problems with the commits in my branch. Closing this PR, will reopen with a clean copy.

@johnkenny54 johnkenny54 deleted the mergeText branch December 29, 2023 15:08
@SethFalco
Copy link
Member

SethFalco commented Dec 29, 2023

Thanks for opening and maintaining the pull request!

When you do have it open, I'll review it once SVGO v3.2.0 is released. Meanwhile, it will probably help if you update with a rebase instead of merge next time!

Assuming origin is your fork and upstream is this repository.

git fetch --all
git rebase upstream/main

Then if there are any conflicts, you can resolve them and do:

git add .
git rebase --continue

This results in a clean commit history, no merges required. 👍🏽
If you ever have trouble because of too many conflicts, it's simpler to squash your commits first and then rebase with upstream after.

git rebase -i HEAD~N # replace N with the number of commits you want to squash
# an interactive editor appears, replace "pick" in all lines except the first with "s"

# now rebase with upstream as normal
git fetch --all
git rebase upstream/main

If you ever have trouble with it again, welcome to ask me to fix it for you, and I'll record how I do it.

@KTibow
Copy link
Contributor

KTibow commented Dec 29, 2023

Thing is that usually merges work fine and don't include upstream commits in the change view, but I think somewhere commit history got changed, resulting in discontinuity across past upstream and current upstream. I ran into this when rebasing another PR I made, and fixed it by git reset --hard HEAD~1ing (deleting the previous merge commit) before retrying the merge and force pushing.

@SethFalco
Copy link
Member

SethFalco commented Dec 29, 2023

Interesting, I always use rebase, so I haven't run into that issue.

commit history got changed

In the past, some maintainers have done force pushes to main, however, that forcefully closes all pull requests that target it. That shouldn't be the problem now.

I'm guessing it's because we use the "Squash and merge" strategy to merge pull requests.

Because this commit is only on the base branch and not the head branch, the common ancestor of the two branches remains unchanged. If you continue to work on the head branch, then create a new pull request between the two branches, the pull request will include all of the commits since the common ancestor, including commits that you squashed and merged in the previous pull request.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/incorporating-changes-from-a-pull-request/about-pull-request-merges#squash-and-merge-your-commits

I, personally, like "Squash and merge" and "Rebase and merge". I'd recommend contributors/maintainers to rebase to keep local branches up-to-date instead of merging.

Looking online, it seems to come down to preference. One way isn't objectively better. I, personally, prefer rebase since I feel merges clutter the commit history. Though, I may consider what's happening now a point for why rebasing is easier.

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.

mergeText plugin to consolidate text/tspan elements Improvement merge text and tspan
4 participants