Suggested changes in code reviewsFreiburg, Germany
I recently wrote that I wanted to blog more often and share how I work. Today’s article is about suggestions in code reviews.
A little more than a year ago, GitHub introduced a button to suggest
reviewing Pull Requests. It’s a neat but somewhat limited feature. I only use
this button as a replacement for (rather confusing) comments like
(which means: “please replace ‘typo’ with ‘fix’”). The GitHub feature does not
work well when I want to suggest a larger change (that could be spread across
About 95% of my code reviews involves checking out the patch proposed in the Pull Request locally. There are many reasons for doing this but I usually make sure a bug fix actually fixes the issue, test the new feature or try to break things (before QA). In some cases, the proposed approach does not seem optimal and I want to see if I can make some improvements. Sometimes, the patch is a “Work in Progress” (WIP) and their author asks for help.
I use hub on top of
git so that I can
checkout a Pull Request using its URL (as shown below).
hub has many more
features and it’s a gem!
$ git checkout https://github.com/user/repo/pull/123
When I want to share the changes I made locally, I use
diff to get a representation of these
pbcopy (on MacOS) to copy the
diff output to the clipboard (I
think we can use
xclip on Linux but I am not sure):
$ git diff | pbcopy
I then go back to the GitHub page and I paste the changes inside a
after one or two lines of text describing the diff and giving some more context:
How about this? Adding `git` again seems too much: ```diff diff --git a/some_file.txt b/some_file.txt index 6b0c6cf..b37e70a 100644 --- a/some_file.txt +++ b/some_file.txt @@ -1 +1 @@ -this is a git diff test example +this is a diff example ```
Anyone who receives such a comment can apply the provided
diff locally. My
current workflow is to copy the
diff to the clipboard and use the set of
commands below to apply it (in reality, I use ctrl + r to
search through my history as it is faster).
$ pbpaste | patch -p1
pbpaste is used to paste content as its name suggests. If you take a closer
look at the previous
git prefixes diff paths with
in the output, which is why
-p1 is needed: it removes
b/ so that
patch finds the right files to patch.
I have been providing suggestions like this for several years now and some folks in my team started to do that as well (and even some contributors!). This workflow is a nice way to share ideas and start discussions. What do you think?