198 lines
8.6 KiB
HTML
198 lines
8.6 KiB
HTML
<!DOCTYPE html>
|
||
<html lang="en-us">
|
||
<head><script src="/livereload.js?mindelay=10&v=2&port=1313&path=livereload" data-no-instant defer></script>
|
||
<meta charset="utf-8">
|
||
<meta http-equiv="X-UA-Compatible" content="IE=edge,chrome=1">
|
||
|
||
<title>My New Hugo Site</title>
|
||
<meta name="viewport" content="width=device-width,minimum-scale=1">
|
||
<meta name="description" content="Pull request practices
|
||
Practices to make code review smoother and more productive.
|
||
Submit your final draft, not your first
|
||
It may be tempting to get your work into code review as soon as possible, but if
|
||
you ask people to read your spaghetti code, then you’ll find that your PR will
|
||
take longer to review, plus reviewers will burn their time and energy asking
|
||
you to fix obvious mistakes instead of checking for more subtle problems.">
|
||
<meta name="generator" content="Hugo 0.145.0">
|
||
|
||
|
||
|
||
<meta name="robots" content="noindex, nofollow">
|
||
|
||
|
||
|
||
|
||
<link rel="stylesheet" href="/ananke/css/main.min.css" >
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
<link rel="canonical" href="http://localhost:1313/programming/general/pull-requests/">
|
||
|
||
|
||
<meta property="og:url" content="http://localhost:1313/programming/general/pull-requests/">
|
||
<meta property="og:site_name" content="My New Hugo Site">
|
||
<meta property="og:title" content="My New Hugo Site">
|
||
<meta property="og:description" content="Pull request practices Practices to make code review smoother and more productive.
|
||
Submit your final draft, not your first It may be tempting to get your work into code review as soon as possible, but if you ask people to read your spaghetti code, then you’ll find that your PR will take longer to review, plus reviewers will burn their time and energy asking you to fix obvious mistakes instead of checking for more subtle problems.">
|
||
<meta property="og:locale" content="en_us">
|
||
<meta property="og:type" content="article">
|
||
<meta property="article:section" content="programming">
|
||
|
||
<meta itemprop="name" content="My New Hugo Site">
|
||
<meta itemprop="description" content="Pull request practices Practices to make code review smoother and more productive.
|
||
Submit your final draft, not your first It may be tempting to get your work into code review as soon as possible, but if you ask people to read your spaghetti code, then you’ll find that your PR will take longer to review, plus reviewers will burn their time and energy asking you to fix obvious mistakes instead of checking for more subtle problems.">
|
||
<meta itemprop="wordCount" content="638">
|
||
<meta name="twitter:card" content="summary">
|
||
<meta name="twitter:title" content="My New Hugo Site">
|
||
<meta name="twitter:description" content="Pull request practices Practices to make code review smoother and more productive.
|
||
Submit your final draft, not your first It may be tempting to get your work into code review as soon as possible, but if you ask people to read your spaghetti code, then you’ll find that your PR will take longer to review, plus reviewers will burn their time and energy asking you to fix obvious mistakes instead of checking for more subtle problems.">
|
||
|
||
|
||
</head><body class="ma0 avenir bg-near-white development">
|
||
|
||
|
||
|
||
|
||
|
||
<header>
|
||
<div class="bg-black">
|
||
<nav class="pv3 ph3 ph4-ns" role="navigation">
|
||
<div class="flex-l justify-between items-center center">
|
||
<a href="/" class="f3 fw2 hover-white no-underline white-90 dib">
|
||
|
||
My New Hugo Site
|
||
|
||
</a>
|
||
<div class="flex-l items-center">
|
||
|
||
|
||
|
||
<div class="ananke-socials"></div>
|
||
|
||
</div>
|
||
</div>
|
||
</nav>
|
||
|
||
</div>
|
||
</header>
|
||
|
||
|
||
|
||
<main class="pb7" role="main">
|
||
|
||
|
||
|
||
<article class="flex-l flex-wrap justify-between mw8 center ph3">
|
||
<header class="mt4 w-100">
|
||
<aside class="instapaper_ignoref b helvetica tracked ttu">
|
||
|
||
Programmings
|
||
</aside><div id="sharing" class="mt3 ananke-socials"></div>
|
||
<h1 class="f1 athelas mt3 mb1"></h1>
|
||
|
||
|
||
|
||
|
||
|
||
|
||
</header>
|
||
<div class="nested-copy-line-height lh-copy serif f4 nested-links mid-gray pr4-l w-two-thirds-l"><h1 id="pull-request-practices">Pull request practices</h1>
|
||
<p>Practices to make code review smoother and more productive.</p>
|
||
<h2 id="submit-your-final-draft-not-your-first">Submit your final draft, not your first</h2>
|
||
<p>It may be tempting to get your work into code review as soon as possible, but if
|
||
you ask people to read your spaghetti code, then you’ll find that your PR will
|
||
take longer to review, plus reviewers will burn their time and energy asking
|
||
you to fix obvious mistakes instead of checking for more subtle problems.</p>
|
||
<p>Do not use code review as a linter. You should not let any mistakes get to code
|
||
review that you could have fixed by scrutinising your own work first.</p>
|
||
<p>Review and polish your own PR before publishing it.</p>
|
||
<ul>
|
||
<li>Have you refactored the code to make it readable and maintainable?</li>
|
||
<li>Have you documented the code where needed? Does the PR description explain
|
||
all of the changes and answer questions the reviewers might have?</li>
|
||
<li>Have you defended your implementation choices?</li>
|
||
<li>Have you reviewed the diff to make sure there are no obvious mistakes?</li>
|
||
<li>Have you looked at the diff in the code review UI and thought about how a
|
||
reviewer might need help to navigate this code?</li>
|
||
</ul>
|
||
<p>You should aim to be the harshest and most thorough reviewer of your own PRs.</p>
|
||
<h2 id="dont-overcomplicate-the-diff">Don’t overcomplicate the diff</h2>
|
||
<p>While you’re working on a ticket you’ll often find other things that need to be
|
||
fixed in the codebase. Some refactoring, a bug, an idea to improve logging etc.</p>
|
||
<p>Do not fix these things in your ticket branch. Because when you publish your PR,
|
||
you’ll have to explain these extra changes to reviewers, or worse, leave them
|
||
guessing about why there are changes in the PR that don’t seem to have anything
|
||
to do with the ticket.</p>
|
||
<h2 id="document-against-huh">Document against “huh?”</h2>
|
||
<p>The behaviour of your code, and the reasoning behind it, isn’t always going to
|
||
be obvious to other devs reading the code, including you when you come back to
|
||
read your code in six months.</p>
|
||
<p>You should always write readable, self-documenting code, but sometimes that
|
||
won’t be enough, and you’ll still need to explain <em>why</em> the code is there in
|
||
the first place.</p>
|
||
<p>For example, a line of code may solve a tricky corner case or do some other
|
||
operation that looks unnecessary at first glance. It’s your job to recognise
|
||
when another dev will look at that code and say “huh?” Answer their questions
|
||
preemptively, by explaining what problem its solving and how.</p>
|
||
<p>If reviewers still ask questions about your code, or make comments that suggest
|
||
that they didn’t understand your code, then that’s often a sign that you still
|
||
need to improve the code’s readability and documentation. If a dev is confused
|
||
about your code now, then there’s a good chance a different dev will be
|
||
confused about that code in future.</p>
|
||
<h2 id="give-reviewers-a-primer">Give reviewers a primer</h2>
|
||
<p>Reviewing code should not be a challenge in puzzle-solving. You want the
|
||
reviewer to understand your changes quickly and easily, so they can decide if
|
||
your changes make sense. This helps them approve your code quickly, or suggest
|
||
improvements to your solution. Both outcomes are good for you!</p>
|
||
<p>Reviewers see your PR changes as a list of diffs sorted by filepath. So if they
|
||
read the diff from top to bottom, they’ll likely be jumping between different
|
||
aspects and levels of your solution, instead of starting at a high level and
|
||
working their way down to the details. Reviewers may need to go over your diff
|
||
several times before it makes sense.</p>
|
||
<p>Use the PR description to explain, at a high level, what problems you solved in
|
||
the ticket and how you solved those problems. If you considered multiple
|
||
solutions, explain why you chose one over the other.</p>
|
||
<p>Your effort on this should be roughly proportional to the complexity of the
|
||
diff and the problems you solved with those changes.</p>
|
||
<ul class="pa0">
|
||
|
||
</ul>
|
||
<div class="mt6 instapaper_ignoref">
|
||
|
||
|
||
</div>
|
||
</div>
|
||
|
||
<aside class="w-30-l mt6-l">
|
||
|
||
|
||
|
||
|
||
</aside>
|
||
|
||
</article>
|
||
|
||
</main>
|
||
<footer class="bg-black bottom-0 w-100 pa3" role="contentinfo">
|
||
<div class="flex justify-between">
|
||
<a class="f4 fw4 hover-white no-underline white-70 dn dib-ns pv2 ph3" href="http://localhost:1313/" >
|
||
© My New Hugo Site 2025
|
||
</a>
|
||
<div><div class="ananke-socials"></div>
|
||
</div>
|
||
</div>
|
||
</footer>
|
||
|
||
</body>
|
||
</html>
|