Pull Requests and Code Reviews - uchicago-bfi-gnlab/lab_manual GitHub Wiki

Creating a pull request

before creation (at the command line)

  • Pull changes from main into your branch.
    • At the command line, checkout branch you will submit a pull request for.
    • Run git pull origin main to merge changes from the remote main branch into your local branch.
    • If there are merge conflicts, open the repo in VSCode and click on the "source control" tab on the sidebar (looks like a forked branch). Merge conflicts will appear at the top. Click on the merge conflicts to see the changes, and select the version you want to commit.
    • Commit changes.
  • Clean code.
    • Refactor the code to produce just the desired outputs and no others.
    • Run styler on all scripts that will be in the PR. After, rerun everything to make sure styler didn't break anything.
    • Commit changes.
  • Check for errors using ChatGPT
    • (At least through June 1 2025) Paste the final version of the code into ChatGPT and ask it to perform a code review. Make any changes you agree with.

Creation (at github.com)

  • Navigate to Pull requests from the top menu, then click new pull request.
  • Change "compare with: issue_XXX_" to be the issue you would like to merge, then click Create pull request.
  • Use the default title created by github. If your branch was called issue_xxx_github_guide the title will be "issue xxx github guide"
  • Pull request attributes
    • Choose a reviewer for the pull request
    • Be added to the relevant project and put in the relevant column on the project board (usually the assigned reviewer's column)

Initial comment (at github.com)

Six-part formula

a) Closes #[ORIGINAL ISSUE NO]"

b) I have reviewed the XXX modified files in the pull request and the changes are as I expected”

c) Indicate the purpose of the pull request as replication vs correctness (more on this in Peer review metadata)

d) State a time estimate for the review (more on this in Peer review metadata).

e) If three or more files are changing, provide an explanation for why several files are changing. Remind the reviewer to check that “important” results (e.g. main figures of a paper) have not meaningfully changed.

f) If there are merge conflicts, provide an explanation for why you were not able to resolve them before creating the PR

Optional Anything else you want the pull request reviewer to know

Peer review metadata

Every pull request should clarify: (i) whether the goal is replication or correctness and (ii) time estimate.

For replication

  • Verify that the submitter ran styler on every script in the PR. If not, ask them to run it and commit the styled code.
  • Verify that the deliverable is clear, complete, and conforms to the standards above
  • Verify that the files committed to the repository conform to our organizational and code style rules
    • It is not typically the job of the peer reviewer to go over every detail of the output and every line of code. While commenting on fine points of code style from time to time is fine, for example, this should not be the primary content of the peer review.
  • Verify that the code runs
    • By default, you should always run the code. If the code could run much more quickly with some edits, suggest those edits and pass back. If are convinced that speed cannot be improved and you don’t run the code, please note this in your review.
  • The peer reviewer should review every plot that has changed.
    • If png, github will show the diff, so every plot should be reviewed.
    • If pdf, then reviewer should open and visually compare every plot to the prior version for any figure in the main paper (or main slide deck, if only a slide deck), but not the appendix or backup plots.

For correctness

  • all of the steps above, plus
    • Compare to written material in the paper or appendix discussing the results
    • Verify that empirical and theoretical results are clear and appear correct
    • Suggest sanity checks to be sure the results are correct

Time estimate

The time estimate is not a maximum or a minimum amount of time to spend. Rather, it is a piece of metadata. If the creator thinks the PR is quick and the reviewer is taking a long time or the creator thinks the PR is complicated and the reviewer thinks it is quick, then this suggests that there has been some miscommunication. Talk live to resolve.

Peer review mechanics

  • When requesting peer review the assignee can request feedback in addition to the above: e.g., particular code or results that need a careful check.
  • Assignees should add a reviewer and add the pull request on the project board to the reviewer. Once the review is complete, the reviewer should move the pull request on the project board to the assignee. The assignee then merge the pull request and move the pull request to the “Done” column.
  • All peer review comments should be made on the pull request itself, not on the original issue.
    • Revision to the files shall be made in the same branch (eg., issue_###_brief_description). The pull request will automatically track changes made in the branch.

After peer reviewer certifies output

Assignee should:

  • Use a squash merge to merge the issue-specific branch into the master branch. If the work is diagnostic in nature and performed entirely within an issues subdirectory, the branch should not be merged.
  • Leave a comment in the task's original GitHub issue with
    • a link to the pull request
    • a summary of the task or a permalink to the /issue_###_brief_description/* output file (or subdirectory) as the final comment.
  • Delete the issue_###_brief_description branch. The permalink will still work.

PI review

  • Assignee closes the task with a message that tags a PI.
  • Assignees moves the task on the project board to a PI column for review. Put this task at the top of the PI column.
  • Once reviewed, PIs move issues to the “done” column on the project board
    • If there is more back and forth on this ticket, the PI will tag the assignee and move the ticket back to the assignee’s column.
    • When the assignee has completed the additional request, they should again tag the PI and move the ticket to the PI’s column.
  • The PI may require changes in the /issue_###_brief_description/* output files. To change the files, pull the issue from the last commit before deleting the issues branch and make a new stable link with the updated files.