docs: clarify pushing to branch etiquette#1436
Conversation
|
|
||
| :::caution | ||
| Do not repeatedly merge the `main` branch into your feature branch. Every merge commit triggers another round of CI runs that we have to pay for, and it clutters your pull request history. When you need to update your branch with the latest changes from `upstream/main`, [rebase](https://git-scm.com/book/en/v2/Git-Branching-Rebasing) instead. Also make sure your existing commits are squashed into a small set of meaningful commits before opening (or updating) your pull request. | ||
| Do not repeatedly merge the `main` branch into your feature branch. Every time something is pushed to the branch it triggers another round of CI runs that we have to pay for, and it clutters your pull request history. You don't usually need to update your branch with the latest changes from `upstream/main`, but if you do it, to minimize the CI runs please push try to this update with other commits that are updating your PR.please bundle the push with other When you need to update your branch with the latest changes from `upstream/main`, [rebase](https://git-scm.com/book/en/v2/Git-Branching-Rebasing) instead. Also, please do not rewrite the history of the commits already reviewed, that makes it harder on reviewers to distinguish new changes from old ones. |
There was a problem hiding this comment.
The language is confusing and incoherent. As a contributor I am not going to bother reading all this let alone understanding it.
If we are needing to amend a caution to be off this length, then our original instructions outside the caution have a gap that we need to address.
Reword, make it concise to fit in one-two lines OR address the underlying instructions.
For example, ask:
- What parts of the process is actually annoying to reviewers?
- Why do we need to preserve previous commits at all (personally I think squashing and rebasing is fine)
- If a follow up changes (seemingly after a review) amended the history, and the reviewer loses context, that's a deeper issue. Are the reviewer then only looking at the delta, and losing track of the whole PR patch – which will land when the PR is merged?, etc.
- Can the instructions be made clear (outside the caution area). Add commits additively, we will squash them. etc.
There was a problem hiding this comment.
what is annoying - getting a notification that is just a merge commit, I guess
the issue with loosing track of the changes is mostly happening when PRs affect whole workshops (adding them or making changes), particularly because these can go on for weeks or months. Checking what was the last feedback left on the PR and being able to look at the "commits since last review" allow to see how the contributor is doing and if they need additional help. It is not strictly necessary, but it allows to see much faster if the PR is ready for an other full review or not.
Smaller PRs don't really need it, but adding too many caveats can be confusing. I will see about clarifying the page.
2 participants
Checklist:
Update index.md)Closes #XXXXX
It wasn't completely on pair with suggestions we give to contributors, so I took the chance to reword it