GitHub And Open Source Collaboration

Pull Requests, Reviews, Merge Methods, And Closing Keywords

A pull request is where Git history becomes a team decision. Local Git can show commits, branches, and diffs, but GitHub adds a review conversation, checks, permissions, linked issues, merge methods, branch deletion, and an audit trail around the change. A good pull request does not merely ask can this code merge? It gives reviewers enough context to decide whether the change is correct, tested, safe to deploy, and worth preserving in the project history.

Use GitHub's official documentation for current interface details. GitHub documents pull request reviews, merging pull requests, and linking pull requests to issues. The stable model is that a pull request connects a head branch to a base branch, collects review evidence, and records how the project accepted or rejected the proposed change.

Pull Request Scope

A pull request should be small enough to review honestly. That does not mean every pull request must be tiny. It means the title, description, changed files, tests, and risk should fit one coherent decision. Fix checkout tax rounding when discounts apply is reviewable. Refactor checkout and update dependencies and redesign invoice email and clean old code is several decisions hiding in one branch.

The description should explain purpose, important design choices, test evidence, linked issues, migration concerns, and what kind of feedback the author wants. A reviewer should not have to reconstruct the business reason from code alone. If the pull request changes a PHP API response, include the before and after contract. If it changes a database query, include the expected query behavior and any migration or index requirement. If it fixes a bug, include the reproduction or failing test that proves the bug existed.

Draft pull requests are useful when the author wants early feedback before the work is merge-ready. They signal that the branch is not asking for final approval yet. This is different from opening a vague pull request and hoping reviewers infer the state. A draft can ask specific questions: Is this boundary in the right service?, Does this migration plan preserve old clients?, or Is this API shape acceptable before I finish tests?

Review States And Review Responsibility

GitHub pull request reviews support statuses such as commenting, approving, and requesting changes. The important habit is to use the status that matches the reviewer's actual decision. A comment-only review is useful for questions, suggestions, or partial feedback that should not block merge. Approval means the reviewer believes the change is acceptable under the project's standards. Requesting changes means the reviewer found something that must be addressed before merge.

Review responsibility is broader than style. A reviewer should look for behavior changes, missing tests, security regressions, data migration risk, compatibility breaks, unclear ownership, and deployment hazards. Style comments are sometimes useful, but they should not dominate a review when automated formatting or linters can handle them. If a PHP pull request changes authentication, payment, serialization, or database state, correctness and failure behavior matter more than a preference for line ordering.

A good review comment explains the concern and, when possible, the standard being applied. This is wrong wastes time. This endpoint returns 200 for validation failure; our API status policy uses 422 with a machine-readable error code teaches the author and gives a concrete fix. Suggested changes can be efficient for small edits, but reviewers should avoid rewriting large parts of the branch inside the review unless the team has agreed that this is appropriate.

Authors also have responsibilities. They should self-review before requesting review, respond to comments directly, push focused follow-up commits, and re-request review when the substance changes. Marking a conversation resolved should mean the concern is addressed or deliberately declined with an explanation. It should not be used to hide disagreement.

Required Reviews And Required Checks

Repository rules can require reviews or status checks before a pull request merges. Those rules are remote quality gates. They are different from local hooks. A local hook helps a developer catch problems before pushing. A required check prevents merge until the repository sees acceptable evidence. For important branches, remote enforcement is what matters because it applies even when a contributor skips local setup.

Required review count is not a substitute for good review. Two shallow approvals are weaker than one careful review by the right maintainer. CODEOWNERS rules can request review from people or teams responsible for specific paths, but ownership still requires judgment. A generated file, migration, deployment script, or authentication boundary may need a reviewer who understands that area even if the path rule is broad.

Checks should prove things reviewers need. A PHP project might require formatting, static analysis, unit tests, integration tests, migrations, security checks, or build validation. Do not require slow checks merely because they exist. Do require checks that catch real release defects. A required check that is flaky trains people to rerun instead of investigate; a missing check trains people to rely on memory.

Merge Methods

GitHub commonly supports merge commits, squash merges, and rebase merges when enabled for a repository. The right method depends on the project's history policy.

A merge commit preserves the branch structure and individual commits. It is useful when the branch contains meaningful commits or when the project wants to show when a feature branch entered the base branch. It can make history more verbose, especially when branches contain many noisy fixup commits.

A squash merge combines the pull request into one commit on the base branch. It is useful when the pull request is the meaningful unit of history and the branch contains small correction commits such as fix typo, address review, or try again. The squash commit message should be edited carefully because it becomes the durable summary. If the pull request mixes unrelated changes, squashing hides that problem rather than fixing it.

A rebase merge replays commits onto the base branch and creates a linear history. It can be useful for teams that value bisectable commits and linear logs. It can also rewrite commit identities and create confusion if authors expect the pull request branch's original commit SHAs to remain the same. Teach rebase merge as a history policy, not as a universally cleaner button.

No merge method fixes weak review. If the code is wrong, the merge button should not be pressed. If the branch is too broad, the merge method will not make it coherent. If the commit messages are misleading, the durable history will be misleading too.

GitHub supports linking pull requests to issues so readers can see that a fix is in progress and so issues can close automatically when a linked pull request is merged. Common closing keywords include forms such as fixes, closes, or resolves followed by an issue reference, subject to GitHub's documented behavior and repository context. Learners should check the current GitHub issue-linking documentation for exact behavior.

Closing keywords are powerful because they connect code acceptance to work tracking. They are also easy to misuse. A pull request that partially investigates a bug should not say Fixes #123 if merging it will not actually resolve the issue. Use Refs #123 or a plain link when the work is related but not complete. Closing an issue early can hide remaining work from maintainers and users.

A good pull request description makes the link explicit: Fixes #123 by validating the coupon currency before applying the discount. Does not change existing tax rounding behavior. That sentence tells reviewers what the branch claims and what it deliberately leaves alone.

Review Flow For PHP Projects

For a PHP application, a strong review flow usually starts with the contract. What input changes? What output changes? What state changes? Which users or systems can observe the result? Then read the tests that prove the new behavior. Then inspect the implementation. Then check the operational path: migrations, configuration, environment variables, cache invalidation, background jobs, API clients, and deployment order.

Reviewers should run or inspect the most relevant checks. They do not need to run every command manually when CI already does, but they should understand what CI proves. If the pull request changes an API status code, tests should prove status, headers, and body shape. If it changes a database query, tests or review evidence should cover empty results, duplicates, permission filters, and performance where relevant. If it changes authentication, review must include threat boundaries and failure behavior.

Authors should keep review easy by separating mechanical changes from behavior changes. Formatting a file and changing authentication logic in the same diff makes review harder. Renaming a class and altering its semantics in the same commit makes history harder to understand. If mechanical cleanup is necessary, split it into its own pull request or commit so reviewers can focus.

Merging And Aftercare

Merging is not the end of responsibility. After merge, delete the branch if the project no longer needs it and if doing so will not disrupt dependent stacked pull requests. Watch the deployment if the change is operationally significant. Confirm linked issues closed only when they should. If a regression appears, decide whether to revert the pull request, roll forward with a fix, or disable the feature. GitHub's audit trail helps only if the pull request description, commits, reviews, and issue links were meaningful.

The learner should be able to open a pull request with reviewable scope, review another person's pull request with concrete feedback, choose an appropriate review status, understand required checks and reviews, explain merge methods, use closing keywords accurately, and treat merge as a recorded project decision rather than a button-click.

Practice

Write a reviewable pull request description

A branch changes a PHP JSON API endpoint so invalid coupon codes return a structured 422 response instead of a generic 400. It also adds tests.

Write a pull request description with:

  • purpose;
  • linked issue using a closing keyword;
  • behavior change;
  • test evidence;
  • reviewer focus.

Acceptance criteria:

  • The closing keyword should only claim the issue is fixed if the described change really completes it.
  • The description must mention status code and response body behavior.
  • The reviewer focus must ask for a specific kind of review.
Show solution
## Purpose

Fixes #184 by returning the documented validation response for invalid coupon codes.

## Behavior change

`POST /api/checkout/coupons` now returns `422` with a JSON error object when the coupon code exists but cannot be applied to the current cart. The previous generic `400` response did not let clients distinguish malformed input from a valid-but-rejected coupon.

## Tests

Added integration tests for expired coupons, wrong-currency coupons, and valid coupons. Existing checkout tests still pass.

## Reviewer focus

Please check whether the status code and error shape match our API status-code policy.

This is reviewable because it states the claim, explains the API contract change, gives test evidence, and asks for focused feedback.

Choose a merge method

For each pull request, choose merge commit, squash merge, or rebase merge. Explain the tradeoff.

  1. A one-commit hotfix with a clear message.
  2. A feature branch with eight noisy commits such as fix typo, address review, and try again.
  3. A library change with four carefully separated commits that should remain bisectable.

Acceptance criteria:

  • Do not claim one merge method is always best.
  • Explain what history the base branch should preserve.
  • Mention that repository policy may restrict available methods.
Show solution
  1. The one-commit hotfix can use merge commit, squash, or rebase depending on repository policy. There is little noisy history to clean up, so the choice is mostly about the project's normal history style.
  2. The noisy feature branch is a good squash-merge candidate if the pull request is one coherent change. The final squash message should describe the feature accurately.
  3. The library change may be a good merge-commit or rebase-merge candidate because the individual commits are meaningful and should remain inspectable. Rebase merge creates linear history; merge commit preserves the branch integration point.

The right answer depends on what history the project wants future maintainers to read.

Review feedback quality
  1. Bad naming.
  2. This seems risky.
  3. Why not just return 200?

The pull request changes an authentication endpoint and adds a new error response.

Acceptance criteria:

  • Each rewritten comment must explain the concern.
  • At least one comment must reference a project policy or expected behavior.
  • At least one comment must ask for evidence or a test.
Show solution
  1. Can we rename this variable from $result to $loginAttempt? This endpoint has several possible outcomes, and the more specific name makes it clearer that the object records one authentication attempt rather than the final session.
  2. This changes the failure path for authentication. Please add a test showing that invalid credentials do not reveal whether the email exists, because our login policy avoids account enumeration.
  3. Why should this return 200 instead of 401 or 422? Our API status policy uses success codes only when the requested operation succeeds. Please document the client behavior that needs 200, or keep the error status consistent with the policy.

The comments are useful because they explain the reviewer's concern and give the author a concrete way to respond.