Allow pulling/testing of merged versions of PRs

[plocket]

When we're reviewing each other's PRs right now it's very hard to train people to do it the way that actually tests the PR properly:

1. Make a new branch with your changes
2. Make a new branch off the base branch
3. Merge those branches
4. Make a PR with that branch on the base branch

Instead, people end up just doing step 1 and reviewers test that branch, which often doesn't actually show you how the PR's changes will behave in the real environment.

Github provides instructions for pulling PRs: https://docs.github.com/en/free-pro-team@latest/github/collaborating-with-issues-and-pull-requests/checking-out-pull-requests-locally

It could be named new_branch_name_112 or old_branch_name_112 or 112_new_branch_name something similar (where 112 stands in for the number of the PR).

[Edit: This could also improve automated/integrated testing.]

[jhpyle]

I don't know what you're asking for, but I'm not sure I understand what the problem is that you are trying to solve. Is it that people's personal branches are too far behind the main branch?

Pretend for the moment that we were all using the command line instead if the Playground. Would it solve your problem if the team members followed these steps?

1. Create their own branch. (git checkout -b john-smith-features)
2. Make changes to their own branch to add new features, and commit the changes. (git add . && git commit -m "added feature")
3. Merge the current version of the main branch into their own branch. (git merge main)
4. Test their changes, making sure that everything makes sense after the merge, and commit again.
5. Push to GitHub. (git push origin john-smith-features)
6. Go to GitHub and create a PR.
7. Reviewer pulls the branch and tests it.

[plocket]

Maybe, but I think it probably wouldn't. I'm trying to think it out and these are the concerns I have so far with that approach:

1. A lot of people don't know how to undo commits, so if that change did something that they didn't want, they would need someone to fix it for them. Automatic changes that would be hard to undo seem something to be cautious of. For that matter, sometimes I would want to avoid updating to the base branch during a PR.
2. A merge conflict would mess things up - either the code or the flow of committing.
3. This thought makes a few assumptions about the proposed feature - we're not always working off of the default branch. People break off into branches off branches and develop there for a while. If they check out a branch we'll call 'development' and start a branch off of that called 'feature1'. They then make PRs to 'development'. Except sometimes they forget and make a PR to the default branch by mistake because that's GitHub's default behavior. It would need more details than merging with the default branch, or even more than merging with the parent branch. Maybe that's not a problem for the implementation you're considering, though.

Also, it occurs to me that an advantage of pulling a PR with a merge conflict would be a much easier and more reliable way to troubleshoot the conflict because you can test your changes as you go.

I don't know enough about the current core code to know what changes would be more difficult to implement. I'd hoped that adding PRs to the 'Pull' dropdown in 'Packages' would be the least invasive. I think it has the chance of being the most useful and least invasive to the development process I've been experiencing. I can try to brainstorm other solutions if you can give me some of the existing constraints.

[jhpyle]

As I understand it, a pull request is a GitHub concept, not a git concept. It is simply a request for someone to merge branch A of repo B into branch C of repo D, where branch A is named something like patch-8. So anyone can "pull a pull request" into the Playground by going to Playground -> Packages -> Pull, entering in the URL of repo containing the "pull request" branch and then selecting the branch (patch-8).

[plocket]

Hmm, I thought a PR contained a version of patch-8 that's already merged with its base branch (thus checking for merge conflicts). If not, then this wouldn't be needed. I'll have to find time to do more research. It would also be a darn shame. At that point, I might ask for a different feature, lol.

[plocket]

I think you're right about PRs, which is really disappointing. I'm sorry this is becoming so complex.

To address the pain point of this issue (will change the title), I have found out that there is a merge sha in the fetch of the PR, though: https://api.github.com/repos/diaspora/diaspora/pulls/8162

{
  ...
  "number": 8162,
  ...
  "merged_at": null,
  "merge_commit_sha": "a04b2fe34b90b42fac305a99b5b24107bf9f33b9",
  "assignee": null,
  ...
}

I can imagine that still leaves open questions.

[plocket]

It would greatly help people collaborate with each other, especially people who are new to git, github, and coding.

[jhpyle]

If a contributor is new to git, github, and coding, you can tell them to just do step 1 and then it would be the reviewer's responsibility to create a temporary branch from the contributor's branch, merge the base branch into it (which can be done in GitHub by creating and immediately merging a PR), and then download that temporary branch into the Playground to test it.

[plocket]

Sometimes both the contributor and the reviewer are new to coding, as when students collaborate on a project.