Skip to content

Lab 1: Collected Code Review Comments

Jump to bottom
Julia Ogris edited this page Jun 12, 2019 · 4 revisions

Thank you for submitting your PR!

A couple high level comments (see below for details):

  • Don't label your PR that is failing CI build as Ready for review
  • Fix your PR description
  • Fix commit messages
  • Limit your number of commit messages to one or two
  • Respond to all review comments with Don. or Rejected because...
  • Don't forget to git pull origin FEATURE-BRANCH after Update branch
  • Don't commit commented out code
  • Don't use global variables that can be scoped local to functions
  • Don't commit binaries
  • Consider using raw strings
  • Consider using a loop instead of recursion
  • Don't capitalise error strings (see official recommendations)

PR description

Please make sure you use proper Markdown (cheat sheet) in your PR description and fill it in accordingly:

  • You need to create a new issue that you reference in your PR description:
    Fixes #<NEW-ISSUE-NUMBER> means it will be automatically closed when the PR is merged.
  • Please also fill in Changes proposed in this PR:

This could look something like:

Fixes #<NEW-ISSUE-NUMBER>

Review of colleague's PR #<PR-NUMBER>

#### Changes proposed in this PR:
- Implement Lab1 Fibonacci numbers
- Test Lab1

Commit messages

https://chris.beams.io/posts/git-commit/

Use git rebase -i COMMIT-HASH to rework your commits

  • Separate subject from body with a blank line
  • Limit the subject line to 60 characters
  • Use the imperative mood in the subject line
  • Do not end the subject line with a period
  • Wrap the body at 80 characters
  • Use the body to explain what and why vs. how

E.g. Change

Lab 1 completed.
Built fib code to binary

to

Complete Lab 1
Build fib code to binary

Limit the number of commits to one or two

You can squash several existing commits into one with git rebase -i COMMIT-HASH and git push -f. For the future, if you have fixup commits use:

git commit --fixup COMMIT-HASH
git rebase -i --autosquash PREVIOUS-COMMIT-HASH
git push -f

Don't forget to pull from origin

Don't forget to git pull origin FEATURE-BRANCH after you have clicked Update branch on github.com. Clicking Update branch merges master into your feature branch FEATURE-BRANCH on the remote. Alternatively to clicking the button you can run locally:

git checkout master
git pull upstream master # or just `git pull` if `master` is set to track `upstream/master`
git checkout FEATURE-BRANCH
git rebase master
git push -f

Consider using raw strings

Consider using raw strings for readability instead of "1\n1\n2\n3\n5\n8\n13\n"

`1
1
2
3
5
8
13
`

Loop instead of recursion

It's probably quite hard to write a recursive solution when printing numbers and not just computing them (you will double print). Instead implement a solution using a for loop.

Clone this wiki locally