Unit 3 · Lesson 4

Pull Requests and Code Review

A pull request is a conversation around code before it reaches main. Every change to Team 2910's robot code requires review — not because we don't trust each other, but because a second set of eyes catches the hardware-destroying bug that the author can't see because they're too close to it.

By the end of this lesson, you will:

Explain what a pull request is and why it sits between a feature branch and main
Write a PR description that gives a reviewer everything they need to evaluate the change
Identify the five categories of issues a reviewer should check in FRC robot code
Write review comments that are specific, actionable, and tied to physical hardware consequences
Recognize the four PR anti-patterns that make review ineffective or impossible

What a Pull Request Is

A pull request — PR — is a formal proposal to merge one branch into another. On GitHub, opening a PR creates a discussion thread attached to the exact diff between your branch and the target branch. Reviewers can comment on specific lines, approve the changes, or request modifications before anything reaches main.

The name comes from the original workflow: you ask the maintainer to pull your branch into theirs. In modern team Git, "PR" and "merge request" are used interchangeably — the concept is the same. What matters is the structured review that happens before the merge, not the button you click to complete it.

For Team 2910, every change to the robot's code goes through a PR reviewed by a mentor or subteam lead before merging. No exceptions during build season. This isn't bureaucracy — it's the single most reliable way to prevent a hardware-damaging bug from reaching a robot that's about to compete.

The Pull Request Lifecycle

A PR moves through five stages from creation to merge. Click each stage to understand what happens and what the author and reviewer are each responsible for.

📃 Open author

🔍 Review reviewer

✏️ Revise author

✅ Approve reviewer

🔗 Merge author

← click a stage to see what happens and who is responsible

Writing a PR Description That Reviewers Can Use

A PR description is not a commit message — it has more room and a different audience. The reviewer hasn't been watching you work. They need enough context to understand the change, evaluate whether it's correct, and know how to verify it works on the robot.

Team 2910 uses a three-section template for every non-trivial PR:

## What changed Added IntakeSubsystem class with motor control and beam break sensor detection. Added intake constants to Constants.java (CAN ID, forward/reverse speeds, stall threshold). Wired updateIntake() call into teleopPeriodic(). ## Why Intake mechanism is now fully wired and mechanically tested. Moving from manual joystick control to a button-toggle with automatic game piece detection. Stall protection added after we burned a NEO during testing on 2024-01-18. ## How to test 1. Deploy to practice robot 2. Enable in teleop — intake should NOT run until A button is pressed 3. Press A — intake should run at ~0.65 forward 4. Trigger beam break with game piece — intake should stop and gamePieceHeld should show true on dashboard 5. Check Shuffleboard: "Intake State" should toggle between IDLE and INTAKING 6. Hold A while stalling the motor — verify current limiting kicks in below 60A

Section 3 — "How to test" — is the one most teams skip and the one that matters most. A reviewer who can't verify the change on hardware has to trust the author completely. Step-by-step test instructions let the reviewer physically validate the behavior before approving, and they serve as documentation for any future programmer who needs to verify the same subsystem works.

PR Description Workshop

The PR descriptions below were submitted without enough context. Select each one to see what information is missing and what a complete version looks like.

PR Description Workshop — select to see the gap click to compare →

01 "Fixed stuff" ▶

02 "Added shooter subsystem. Ready for review." ▶

03 (no description — 34 files changed) ▶

04 "Tuned PID gains for arm. kP=0.08 kI=0 kD=0.002" ▶

❌ This description

✅ Complete version

Reviewing Code: What to Actually Look For

A good review isn't "does this look fine to me." It's a systematic check across categories that experience shows are the most common sources of competition failures. Team 2910 reviewers check five things, in this order:

Hardware safety. Does this change risk damaging a motor, servo, or mechanical component? Unconstrained outputs, missing current limits, unsafe initialization values.
Correctness. Does the code do what the PR description says it does? Are there obvious logic errors, wrong comparison operators, or missed edge cases?
Constants and naming. Are all numerical values named constants in Constants.java? Are variable and method names descriptive enough to understand without comments?
Scope and side effects. Does this change touch more than it claims to? A PR titled "add intake subsystem" that also edits the drivetrain is a scope problem.
Testability. Are there testing instructions? Is the change small enough to verify on hardware?

PR Review Simulator

Each scenario below shows a diff from a real-style FRC PR and a review comment an experienced reviewer would leave. Select any scenario to read the diff and see what the reviewer caught — and why it matters on a physical robot.

PR Review Simulator — select a diff to see the review click to review →

01 IntakeSubsystem.java — motor output set as a magic number ▶

02 Robot.java — blocking while loop added to teleopPeriodic() ▶

03 ClimberSubsystem.java — new motor added with no current limit ▶

04 Constants.java — new CAN ID duplicates an existing value ▶

05 ArmSubsystem.java — boolean state variable initialized to true ▶

🔍 Review comment

✅ What to change

Writing Review Comments That Help

The goal of a review comment is to give the author enough information to fix the problem themselves — ideally without a back-and-forth conversation. A useful review comment has three parts:

What the issue is — specific enough to find the line
Why it matters — what goes wrong on the robot if it stays this way
A suggested fix — not required, but dramatically accelerates resolution

❌ Unhelpful: "This needs to be changed." ❌ Unhelpful: "Magic numbers are bad." ✅ Helpful: Line 24: motor.set(0.8) — this should be a named constant in Constants.java. If a mentor asks "what speed is the intake running at," we need to be able to answer without grepping the codebase. Also, 0.8 on this motor may exceed the stall current limit when the intake is jammed — what value was tested? Suggestion: static final double INTAKE_SPEED_FORWARD = 0.75; in Constants.java, verified against the 40A breaker on your PDH slot. ✅ Helpful: Line 31: while (!beamBreak.get()) { } — this blocks teleopPeriodic() until the sensor triggers. If it takes more than 20ms, the Driver Station sees a loop overrun and may disable the robot. I've seen this exact pattern take down a robot mid-match when the sensor cable was loose. This needs to be refactored as a state check — see Lesson 5 for the pattern.

💡 Comment tone: critique the code, not the person

"This is wrong" and "you made a mistake" are different statements. Review comments should always be about the code, not the author. "This magic number should be a named constant" is correct. "You should know better than to use magic numbers" is not. The distinction matters especially on a student team — code review should be one of the best learning experiences in the curriculum, not a discouraging one.

Draft PRs: Work in Progress That Needs Early Eyes

GitHub lets you open a PR as a Draft, which signals that the work isn't ready to merge yet. Draft PRs are useful for:

Getting early structural feedback before writing all the implementation code
Sharing work-in-progress with a mentor who wants to stay informed
Triggering automated checks (CI) on code that isn't finished yet
Preserving a branch in the PR queue without accidentally triggering a merge

The 2910 convention: open a Draft PR when you want eyes on direction, not a final approval. Convert it to Ready for Review when the work is complete and you've done your own self-review.

🔍 LRI Observation

The most effective change I've seen a team make to their code process was requiring every programmer to do a self-review before requesting someone else's time. That means reading your own diff in the GitHub PR interface — not in VS Code where you're too familiar with the code — before marking it ready. Reading your diff as a reviewer, not as the author who just wrote it, catches roughly half the things a second reviewer would catch. Draft PRs make this natural: write the code, open a draft, review your own diff, fix what you catch, then convert to ready.

Four PR Anti-Patterns That Break the Process

34 files changed The Monolith PR

A PR that touches everything can't be reviewed effectively — a reviewer can't hold 34 file changes in their head at once. It also can't be reverted cleanly if part of it breaks something. Split large PRs into focused, independently reviewable pieces: one for the subsystem skeleton, one for the PID implementation, one for the dashboard integration.

LGTM The Rubber-Stamp Review

"Looks good to me" without line-level comments means the reviewer approved without reading. This is worse than no review — it gives false confidence that the code was checked when it wasn't. If a blocking while loop survives a rubber-stamp review and disables the robot, the reviewer shares responsibility for the outcome.

(no description) The Context-Free PR

A reviewer without context can only evaluate whether the code compiles and looks syntactically reasonable. They can't evaluate whether the behavior is correct, whether the testing was adequate, or whether this change conflicts with something in another subsystem. No description means no meaningful review.

Reviewed my own PR Self-Review

A programmer cannot effectively review their own code for logical correctness — they know what they intended the code to do, so they see what they meant rather than what it actually does. The entire point of code review is a second perspective. A reviewer who is also the author defeats the purpose entirely.

🔍 Event Observation

At a regional I reviewed code for a team that had been using PRs all season. Their process: create PR, wait fifteen minutes, approve it yourself, merge. They had the form without the substance — a complete PR history with zero meaningful review comments across forty-something PRs. Their autonomous broke in quarterfinals because a constant that should have been tested was never questioned by anyone except the programmer who added it. Review processes only work if reviewers actually read the code. The ceremony of the PR is worthless without the attention of the reviewer.

🔌 System Check

⚙️ Code Review Standards for Competition Readiness

Every PR that touches hardware configuration requires a hardware-aware reviewer. A reviewer who doesn't know what a stall current limit means cannot evaluate whether a current limit value is safe. At minimum, one reviewer per PR should understand the physical mechanism the code controls.
PRs that touch Constants.java get extra scrutiny. Constants control physical behavior — speeds, limits, CAN IDs. A wrong constant value can be more dangerous than a logic error because it affects the robot silently with no exception or error message.
No PR merges without testing instructions in the description. "Deploy and verify it works" is not a testing instruction. Step-by-step hardware verification steps, with expected sensor readings or dashboard values, are the minimum.
Self-review before requesting others. Read your own diff in the GitHub PR interface. Fix what you catch. This is your responsibility as the author — not the reviewer's.
Review comments are logged forever. A record of what was caught, what was changed, and why provides institutional memory that survives programmer turnover. A team whose PRs have substantive comments is a team that can onboard a new programmer without losing context.
At competition, PRs may need to be fast-tracked. Designate a process for time-sensitive changes — a ten-minute pit review is better than no review, but "I'll review it after the match" means after the match is when you find out if it was wrong.

Knowledge Check

Click an answer to check your understanding.

A reviewer opens a PR and writes "LGTM — approved" after reading the description but without opening the Files Changed tab. What is the problem with this review?

1LGTM is not a valid approval — GitHub requires a formal Approve action
2The reviewer didn't leave enough comments — GitHub requires at least three per review
3The reviewer evaluated the description of the change but not the actual code — a description can be accurate while the implementation contains hardware-damaging bugs that only appear in the diff
4This is acceptable — reading the description is sufficient for small PRs

A PR titled "add shooter subsystem" has 28 files changed, including edits to the drivetrain, the intake, and a refactor of Constants.java. What is the core problem with this PR, and what should the author do?

128 files is too many for GitHub to display correctly — the PR should be split to avoid rendering issues
2The PR title is misleading — rename it to accurately reflect all changes
3The PR contains multiple unrelated concerns — a reviewer cannot meaningfully evaluate all of them at once, and a revert would undo changes across three different subsystems; the author should split it into focused, independently reviewable PRs
4No problem — large PRs are normal in FRC and reviewers are expected to handle them

A reviewer sees this line in a PR diff: armMotor.set(0.9); in a method that runs every periodic cycle. What should their review comment address, and in what order?

1Only the naming convention — 0.9 should be stored in a variable
2Nothing — 0.9 is within the valid −1.0 to 1.0 range for motor controllers
3First the hardware safety question: was 0.9 tested? Does it respect the current limit and breaker rating for this motor? Then the constants issue: this literal should be a named constant in Constants.java. Both concerns exist; hardware safety comes first.
4Ask the author to add a comment explaining what 0.9 means

💪 Practice Prompt

Open, Review, and Revise a Real PR

This prompt walks through a complete PR cycle using the robot project you have been building in the Lesson 2 and 3 practice prompts. It requires two people — an author and a reviewer — or you can play both roles using two browser windows.

Author: create a feature branch. Run git switch -c feature/add-arm-subsystem. Create a minimal ArmSubsystem.java file with at least one motor, one constant, and one method. Include at least one intentional problem from this lesson: a magic number in a method call, a boolean field initialized to an unsafe default, or a variable with a vague name. Commit and push the branch.
Author: open a PR on GitHub. Navigate to your repo, click "Compare & pull request" on your branch. Write a complete three-section description using the template from this lesson: What changed / Why / How to test. Set the target branch to main. Request your teammate as a reviewer.
Reviewer: open the Files Changed tab. Read the diff carefully using the five-category checklist from this lesson: hardware safety, correctness, constants and naming, scope, testability. Leave at least three line-level review comments. For each comment, include: what the issue is, why it matters on a robot, and a suggested fix. Submit the review as "Request Changes."
Author: address the feedback. For each review comment, make the requested change on your branch, add a commit (with a proper message referencing what the review caught), and push. Reply to each comment on GitHub explaining what you changed and why.
Reviewer: re-review and approve. Verify that each requested change was made correctly. If the fixes are acceptable, submit an Approve review with a substantive comment explaining what you verified — not just "LGTM."
Author: merge and clean up. Merge the PR using the "Squash and merge" or "Merge commit" option (discuss with your mentor which your team prefers). Delete the feature branch from GitHub. Pull main locally and run git log --oneline -5 to confirm the merge appears in the history.
Bonus: Look at three merged PRs from Team 2910's most recent season repository on GitHub. For each one, answer: Does the description have all three sections? Are there line-level review comments? Can you tell from the PR alone whether the change was physically tested? Write one paragraph summarizing what their PR process communicates about how the team works.