From 0a3c50617b416737b381a8b65ed9929d0acfcd2c Mon Sep 17 00:00:00 2001 From: Thomas Date: Sun, 17 Apr 2022 11:07:40 +0100 Subject: [PATCH] Add 'why did this PR take so long' blog post --- README.md | 38 +------------ package.json | 2 +- .../blog/2022-04-17-why-is-this-pr-so-big.md | 56 +++++++++++++++++++ src/routes/__layout.svelte | 1 + src/routes/blog.svelte | 24 +++++++- src/routes/blog/[slug].svelte | 4 ++ src/routes/index.svelte | 4 ++ src/styles/thomaswilson.css | 2 + 8 files changed, 93 insertions(+), 38 deletions(-) create mode 100644 src/content/blog/2022-04-17-why-is-this-pr-so-big.md diff --git a/README.md b/README.md index 374efec..b20a1a3 100644 --- a/README.md +++ b/README.md @@ -1,38 +1,4 @@ -# create-svelte +# Thomas Wilson -Everything you need to build a Svelte project, powered by [`create-svelte`](https://github.com/sveltejs/kit/tree/master/packages/create-svelte). +The source code for `thomaswilson.xyz` written in SvelteKit -## Creating a project - -If you're seeing this, you've probably already done this step. Congrats! - -```bash -# create a new project in the current directory -npm init svelte - -# create a new project in my-app -npm init svelte my-app -``` - -## Developing - -Once you've created a project and installed dependencies with `npm install` (or `pnpm install` or `yarn`), start a development server: - -```bash -npm run dev - -# or start the server and open the app in a new browser tab -npm run dev -- --open -``` - -## Building - -To create a production version of your app: - -```bash -npm run build -``` - -You can preview the production build with `npm run preview`. - -> To deploy your app, you may need to install an [adapter](https://kit.svelte.dev/docs/adapters) for your target environment. diff --git a/package.json b/package.json index 4ea5b24..7501887 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "thomaswilson-sveltekit", - "version": "0.0.1", + "version": "1.0.0", "scripts": { "dev": "svelte-kit dev", "build": "svelte-kit build", diff --git a/src/content/blog/2022-04-17-why-is-this-pr-so-big.md b/src/content/blog/2022-04-17-why-is-this-pr-so-big.md new file mode 100644 index 0000000..a52f0f1 --- /dev/null +++ b/src/content/blog/2022-04-17-why-is-this-pr-so-big.md @@ -0,0 +1,56 @@ +--- +title: "'How did this PR get so big?' – Advice for separating aesthetic and functional changes in code" +author: "Thomas Wilson" +date: 2022-04-17T10:48 +slug: "2022-04-17-why-is-this-pr-so-big" +draft: false +tags: + - engineering +--- + +A few weeks ago I was working on some change in our codebase. One thing lead to another and the diff I submitted spanned eighty-some files, and over a thousand lines of modified code (adds, removes, and modifies). + +The first question I was asked in the sync code review was "how did this PR get so big?". The short answer is that I mixed functional and aesthetic changes into the codebase: + +- **A functional change:** changes the behaviour of the system, by adding functionality or modifying existing behaviour. This might include adding a field to an API response, or some framework changes to database persistence. +- **An aesthetic change:** changes how the code looks or feels, without altering functionality. This is refactoring, or ergonomic changes. It might look like renaming something, moving functions around. + +It's a good question, though ("why is this PR so big?"). It implies that smaller, more frequent changes to the codebase, and subsequent automated deploys, are the goal (they are) and that the natural enemy of this goal is large, many-file changes (it is). That's mostly because they consume human attention, which is finite and valuable. + +PRs (pull requests) represent a change the team wants to make to the codebase. It's common practice that you have someone double-check the changes to the code _before_ the change goes to the `main` branch (a code review), and less-common, but higher value imo, to have someone double check your workings as you go (pairing / mobbing). I previously thought that PRs represented a feature, but I don't think they do anymore. I think they represent a safe change to the codebase in the direction of a feature. + +As a rule of thumb, more regular changes to the codebase is better. Big PRs make regular changes more effort, so probably decrease their frequency. The alternative is less diligent review (easy), or higher reliance on tests (hard). + +## So just make functional changes, then? + +You can't just _not_ do either of these kinds of changes, though. Either your code will never add new features (your product team and users hate you) or your code is horrible to work with and speed of iteration slows down (your engineers and users hate you). + +They're both parts of the code rainforest, like bugs and fungus. The problem is that aesthetic changes take quite a bit of human understanding and produce quite a bit of visual noise to a diff. If something's been renamed globally, there could be a lot of files affected. Likewise if you edit the arguments to a function, or move something from one place to another. + +In a visual diff (like you get on GitHub or BitBucket) these kinds of changes are given just as much visual weight as adding a function or changing its name. It's up to the human doing the review to recognise this as noise over signal. + +## Okay, so do them both but separately + +I want to work on stricter segregation of functional vs. Aesthetic changes in a PR. Martin Fowler ([in his book *Refactoring*](https://martinfowler.com/books/refactoring.html)) would encourage us to i) plan the change, ii) refactor to make the change easily, and iii) make the change. + +Pragmatically, and I'm sorry, Martin, I find it easier to recognise a lot of the necessary aesthetic changes _while_ I make the functional ones. That's why I code with a notepad in front of me, to write down things I need to change or things which pop up. + +This has the added values of: + +- More immediate attention goes to solving the functional change than the aesthetic one. +- Conversely, if immediate attention cannot effectively be poured into solving the functional problem, you've got a strong signal that you need some aesthetic changes. +- Sometimes writing down a grievance with the code is enough to get my frustration out. I've said the thing's bad, I've recognised why, but I've got to move on. +- Putting time between thinking "I should refactor this" and "I am going to refactor this" filters out unimportant changes. +- You get a notebook full of largely indecipherable comments. I review this every morning and at the end of every week, to refresh the pains I felt and ideas I had. + + +## What now? + +You can separate out your work via commit or via PR. I'd advise by PR, because you can pick different reviewers for each. That can spread knowledge about your codebase, and about general refactoring principles. This can be really helpful for more junior engineers. + +When opening multiple PRs, you will have to address the visceral feeling of shipping a feature to production and leaving your little JIRA card in "in progress". I'm sorry. If you've got feature-focused product teams or management, you might also find this a politically difficult move. In which case: open a PR with changes stored by commit. You can encourage reviewers to review a PR by commit, and do a mob review if you want to share the knowledge. + + + + + diff --git a/src/routes/__layout.svelte b/src/routes/__layout.svelte index c7d818b..a6b63e8 100644 --- a/src/routes/__layout.svelte +++ b/src/routes/__layout.svelte @@ -2,4 +2,5 @@ import "../styles/thomaswilson.css"; + diff --git a/src/routes/blog.svelte b/src/routes/blog.svelte index db5c5ff..c3456a0 100644 --- a/src/routes/blog.svelte +++ b/src/routes/blog.svelte @@ -42,6 +42,10 @@ export let daysSinceLastPublish: number; + + Blog | thomaswilson.xyz + +
@@ -51,7 +55,7 @@ things.

- I like to write at least once a month. It's been + I like to write at least once a month. It's been {daysSinceLastPublish} {daysSinceLastPublish === 1 ? 'day' : 'days'} since I last published something. @@ -125,4 +129,22 @@ padding: 8px; font-family: monospace; } + + .days-since-success { + color: var(--brand-green); + border: 1px solid var(--brand-green); + animation-name: pulse_green; + animation-duration: 5.2s; + animation-iteration-count: infinite; + background: rgba(54, 130, 127, 0.05); + } + + @keyframes pulse_green { + 0% { + box-shadow: 0 0 0 0px rgba(54, 130, 127, 1); + } + 20%, 100% { + box-shadow: 0 0 0 5px rgba(54, 130, 127, 0); + } + } diff --git a/src/routes/blog/[slug].svelte b/src/routes/blog/[slug].svelte index a2c3d47..57c9d9f 100644 --- a/src/routes/blog/[slug].svelte +++ b/src/routes/blog/[slug].svelte @@ -31,6 +31,10 @@ export let post: Post; + + {post.title} | thomaswilson.xyz + +

diff --git a/src/routes/index.svelte b/src/routes/index.svelte index 3af13ba..dd224b8 100644 --- a/src/routes/index.svelte +++ b/src/routes/index.svelte @@ -10,6 +10,10 @@ let isPersonalExpanded = false; + + Thomas Wilson + +
diff --git a/src/styles/thomaswilson.css b/src/styles/thomaswilson.css index b09590e..65c9d3f 100644 --- a/src/styles/thomaswilson.css +++ b/src/styles/thomaswilson.css @@ -85,6 +85,8 @@ h6 { font-weight: 700; margin: 0; color: var(--gray-800); + padding-top: 8px; + padding-bottom: 6px; } p,