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

Refactor merge script into classes, don't require rebase #3276

Merged
merged 1 commit into from
Jan 30, 2021

Conversation

HebaruSan
Copy link
Member

@HebaruSan HebaruSan commented Jan 24, 2021

Background

In #3263 we created a Python script to merge pull requests more easily.

Motivation

The script is very procedurally-oriented; it's one long function containing a sequence of complex steps that aren't always clearly related to one another, and not much structure or abstraction. This makes it harder to isolate one particular operation and check that it works properly.

Problem

The script seems unable to merge branches that aren't rebased on current master, even if the conflicts are trivial. It is inconvenient to have to rebase every pull request before merging.

Cause

repo.index.merge_tree doesn't do well with conflicts, but it's what you'll find if you go looking for ways to merge with GitPython. I think it is intended to be a low-level tool for apps that intend to re-implement parts of the git merge command themselves differently. That's not us; we just want the regular merge command.

Changes

  • New class CkanRepo represents the repo on disk and handles common operations such as checking if the master is up to date or editing files or knowing where the change log is
  • New class CkanPullRequest represents a pull request and handles other operations such as getting the change log entry and commit message and doing the actual merge
  • The merge steps now use repo.git.merge, which handles trivial conflicts better, and repo.git.commit, which is needed when the merge is started with repo.git.merge. These are simple wrappers around the git merge and git commit commands, so we can be more sure they'll be similar to the old commands.
  • Also added automatic generation of the change log entry badge based on the labels; it translates them from label to badge, and if only one is recognized, it uses that, otherwise it uses Multiple for multiple and UNKNOWN for none, to be filled in by the user

@DasSkelett
Copy link
Member

One thing I didn't catch in #3263, the packages are called gitpython and pygithub. Amended a change renaming them and reordering the dependencies alphabetically.

Since repo.git.merge is just a wrapper, it should also handle deleted files correctly now, I guess. There was no open PR with deleted files to test.

Copy link
Member

@DasSkelett DasSkelett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, makes it more readable, too.

@HebaruSan HebaruSan merged commit 9a7e4fe into KSP-CKAN:master Jan 30, 2021
@HebaruSan HebaruSan deleted the feature/pr-merge-py-objs branch January 30, 2021 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants