Skip to content

Branches, PR

Mike S edited this page Jan 15, 2018 · 3 revisions

Branches:

  • master = Default branch (development in git-flow), is automatically deployed to dev stage.
  • deploy/production, deploy/beta, deploy/<stage> - Automatically deployed, should be only ff to previous stage or master branch
  • hotfix/<name> - For critical hotfixes, should be merged to all stages.
  • pr/<name> - For branches that should end in PR (feature branches), should be merged into master. Don't be afraid to use longer descriptive names with hypes like pr/impleenting-this-fucking-boring-feature, if the branch is addressing specific issue, append issue number at the end e. g. pr/new-messaging-32

PR workflow

  • When implementing new issue, you should create your own pr/<feature> branch, and open PR on github after first push. Prefix PR name with [WIP] (Work in progress). And assign it to yourself.
  • The branch is yours, or whoever you taked to, you are allowd to force-push there.
  • In PR (even in not finished yet) describe what are you trying to accomplish.
  • Before review make sure:
    • you have written test for your features
    • your PR is passing on CI
    • your PR have note explaining what you have done.
  • When you think you're done, remove the [WIP] tag, and ask someone to review, either
    • Ask them personally (FB, Slack, Email, etc.), and when they agree assign them to the PR
    • Prefix PR name with [RR] (Review request), someone shoul then go and review your code.
  • We are using https://reviewable.io/ instead of GitHub reviews. So don't assign people as review on GH
  • When someone reviews PR, unassignes itself, and assignes PR owner to either merge or address issues, or someone else, if he thinks, someone else should see the code as well, when PR owner address issues review process repeats.
  • If you don't have push access to repository, ask in PR to merge.
  • Whoever is assigned is the one who should take action (review, fix bugs, merge, etc.)

Code review rules

  • Reviewer does not have to be better programmer than author, the main purposes of CR are:
    • Somebody else should see the code for obvious errors (typos etc.)
    • At leasts two people should understand the code.
    • Learn! CR are the best way to learn how to program.
  • Do not be afraid to ask!
  • Read https://blog.scottnonnenberg.com/top-ten-pull-request-review-mistakes/

For reviewers:

  • Read everything, tests, migrations, even commit messages
  • Dont be afraid to point out simple or dumb things

For authors:

  • Don't take it personally! Your code is not your child, when somebody criticised your code, he doesn't mean to hurt you.
  • Try to think about every comment.

Merging and fixuping

  • Rebase, rebase, rebase
  • After CR (code review) there are typically two kinds of issues
    • Little issues, mainly typos, code formating. Those should be fixed as fixups into ther original commits, to have clean history, or edited during rebase. The should be no commits like code style fix or fixup typo
    • Bigger code changes, that usually need new commits, don't be afraid to rebase, swap commits, edit messages, it's your work, you can force-push and make the history clean before is merged.
  • PR need to be up-to-date with base branch before merging, do that by rebasing your branch on base branch. Typically git rebase --interactive --autosquash origin/maste, do not use GitHub button Update branch, that will just merge base branch into yours.
  • Merge is done on Github with button, where you need to pass all checks, do not merge and push on your computer. Merge is done mainly by PR author if it's posible, by admin/contributor otherwise.
  • After merge, branch should be deleted (it's you a pointer anyway)

Clone this wiki locally