[HN Gopher] Quick fixes to your code review workflow
___________________________________________________________________
Quick fixes to your code review workflow
Author : hyperpape
Score : 76 points
Date : 2022-05-20 13:28 UTC (1 days ago)
(HTM) web link (consulting.drmaciver.com)
(TXT) w3m dump (consulting.drmaciver.com)
| fishywang wrote:
| A better fix is to abandon the whole "pull request" model used by
| github (and copied by a lot of others, like gitlab, etc.), and
| use a proper code review system (gerrit, etc.) instead.
|
| But of course that's not a "quick" fix.
| yodsanklai wrote:
| What's the difference?
| fishywang wrote:
| A lot. Here are some examples:
|
| Say you are the reviewer, you gave some feedback for the
| author to address. The author says they addressed them and
| asked for your review again. How do you review that? Of
| course you can just review the whole change again, but if
| it's a huge change and you only asked a few small parts to be
| changed, it would be hard to make sure the rest are still the
| same. How in Github's PR model can you see the diff between
| the 2 states of a PR? The only way is for the author to push
| a separated, "fixup" commit to the review branch, so you can
| just view the diff of that commit. But that creates those
| "fixup" commits that's not useful in the final merge, so you
| will want to use squash merge in the end, and using squash
| merge means that the final commit message is totally up to
| the people clicking the merge button and others have no
| control over that. In gerrit, commit messages are part of the
| code that you can review and leave feedback, a change is
| always a single commit, and there's builtin feature to show
| diff between the 2 states of a change.
|
| Another example is when you have a change (B) depending on
| another pending change (A), and there are some feedback on
| change A you need to address, and it would be a whole mess on
| how you also update change B to make sure that changes of A
| do not show up in the diff of B.
| dottedmag wrote:
| https://reviewable.io handles the rebases in PRs pretty
| well, and is a tool that works with GitHub PRs.
| SAI_Peregrinus wrote:
| GitHub marks specific files as having been changed since
| the last time you viewed a PR. Sadly not specific lines
| though.
| Stratoscope wrote:
| > _How in Github 's PR model can you see the diff between
| the 2 states of a PR?_
|
| This is super easy with SmartGit and it works with any Git
| host.
|
| The Branches panel lists both my local branches and all
| remote branches. I double-click the remote branch and it
| offers to create a local branch for it.
|
| Then I can compare any two commits by simply clicking one
| of them in the Graph (log) panel and Ctrl+click the other
| one I want to compare. Whenever two commits are selected,
| the Files panel lists the files that changed between the
| two.
|
| Click any of those files and the Changes panel shows the
| changes between the two commits. Double-click the file to
| open a more full-featured compare tool such as Meld.
|
| The Branches panel has a couple of other great features: a
| list of your stashes, and a Recyclable Commits checkbox.
| When you turn on any of those, their commits appear in the
| Graph panel just like any other commit.
|
| So if you've really messed up, instead of having to trawl
| through the reflog and checkout a commit you think might
| have the code you need, you can just click around and view
| any reflog commit or compare any two commits. Same for
| stashes. Instead of all these special cases, every kind of
| commit is unified into one consistent model.
|
| https://www.syntevo.com/smartgit/
| g4zj wrote:
| _Say you are the reviewer, you gave some feedback for the
| author to address. The author says they addressed them and
| asked for your review again. How do you review that?_
|
| I can't speak for GitHub, but Git _Lab_ allows you to
| easily compare previous versions (states) of a branch.
| Whether you push additional commits or squash them locally
| and force push to update the remote branch, the differences
| between versions are easy to compare.
|
| https://docs.gitlab.com/ee/user/project/merge_requests/vers
| i...
| hipnoizz wrote:
| I've spent probably close to a decade using Gerrit with
| various sized teams, and in the past few months I had an
| opportunity to work with Gitlab (so the MR/PR model). I've
| found that I definitely prefer Gerrit (and this feeling is
| shared by my colleagues). I think it boils down to following
| reasons.
|
| Gerrit is a CR tool only (OK, it also hosts you repositories)
| and I've found that its UI allows me to focus on the code
| being reviewed much better than Gitlab. There are multiple
| maybe small things (navigation patterns, keyboard shortcuts,
| they way the information is provided) but all together make a
| tangible difference, one of reason being that reviewing
| someone other code is not an easy one and any obstacle will
| make it more mundane and lead to very shallow reviews.
|
| I've found that a commit-based review process (although it is
| not enforced, i.e. a developer can push whatever number of
| commits and then ask for a review/assign reviewers) is much
| (much) better than the PR/MR model. I think it nudges
| developers into a) more though-out commits, b) smaller
| commits, c) pushing early and getting early feedback.
| Probably the fact that we are working with a bit
| unnecessarily complex branching model now makes things worse,
| but even without that I think that an effort to push changes
| (especially smaller ones/small fixes/small improvements) is
| (much) smaller in Gerrit.
|
| There is one oddity with Gerrit, namely so-called 'Change-Id'
| (the way Gerrit tracks new revisions) - nothing unmanagable,
| but it probably will make you trip a few times at the
| beginning. And likely you will need to learn Git a little
| better (in general you will need amend/rebase to apply
| changes after your colleagues commented on them). If you are
| using any of JetBrains IDEs there is a very good plug-in,
| which also allows to easily fetch changes you are asked to
| review. All in all in the past decade we were able to on-
| board all new hires without much pain.
|
| I still like many other bells and whistles provided by Gitlab
| (and likely Github), especially build pipelines. I would like
| to find some time to try to 'integrate' Gerrit with Gitlab -
| Gerrit can easily push changes to other Git repository, so I
| can imagine it would be possible to plug into build pipelines
| etc. etc. Yeah... so many things to do, so little time ;-)
|
| Edit: One more thing - it is mentioned by fishywang
| below/around - I've just read his comment and facepalmed
| about myself why I didn't listed it as well - tracking
| comments/discussion/changes/revisions/diffs - this may be the
| biggest advantage of Gerrit - really, maybe I don't know how
| to use Gitlab properly, but this aspect there is really weak.
| jimmont wrote:
| A tremendously helpful article. Possibly one of the best HN
| shares I've seen related to software. Most code reviews I've seen
| professionally over the past decade are almost entirely stylistic
| one-way brittle dialogs over text of small to large pull/change-
| requests. When in reality these could, should, recommended to be
| (as the article shares) side-by-side discussions for dialog to
| improve the product being contributed to. It's really an
| effective way to ensure that someone else can pick up the work
| and go if the other gets hit by a bus, and in my own experience,
| when I lead with an explanatory walk-through exposes bugs (to
| myself) along the way--that even the reviewer often doesn't note.
| Thanks for sharing, software development practice is several
| decades behind... other practices, and content like this help to
| improve it.
| vinnymac wrote:
| I rarely see this mentioned, but one thing that has made a huge
| improvement to my day to day, is enabling scheduled reminders for
| code reviews. I have it configured with email and Slack right
| now, and never miss a review. Rather than having to keep tabs on
| the state of reviews, or synchronously discuss a review request,
| I can just wait until I get a review request in my notification
| tray. It's been the most seamless experience I have found so far.
|
| I am not sure if Gitlab or Bitbucket offer similar functionality,
| but you can configure it here on Github.
| https://github.com/settings/reminders
| nurgasemetey wrote:
| I quickly improved my PRs and sped up code reviewing by doing
| following when I create PRs
|
| - make a diagram if is possible
|
| - make a summary of changes like why this file changed and
| decision behind this
|
| - make a note about backward compatibility
|
| - anticipate which questions can be asked and answer them upfront
| thinkingkong wrote:
| One of the biggest improvements Ive made and rolled out was to do
| my own review first. Scan through all the code from the reviewers
| perspective and try and see where adding a comment, renaming
| something, or explaining a decision might be beneficial. It makes
| life way easier for everyone.
| franzb wrote:
| Absolutely! This works even better when doing the self-review
| with the same tool (typically, GitHub review system) as the one
| a real reviewer would use. Just looking at one's code in a
| different environment (different font, colors, etc.) helps
| switching from a "programmer's mind" to a "reviewer's mind".
| notemaker wrote:
| I, too, need to review my code in Gitlab/Github/Gerrit in
| order to catch errors - somehow they're much more prominent
| there (like you said, I suppose it's due to adopting a
| "reviewer's mindset") rather than just looking at `git diff`.
|
| But if possible I would like to review my work in the
| terminal as a part of my workflow, not needing to context
| switch to the browser. Has anyone found a good solution to
| that?
|
| FWIW, using tmux & nvim.
| AndrewDucker wrote:
| Totally. Looking at the diff and asking "But why did I make
| this change? And how would I justify it?" is very helpful.
| vinnymac wrote:
| This is actually a requirement on my team. If someone does not
| do a self review, I leave a comment explaining that I won't be
| reviewing the work until the decisions that were made are
| explained.
|
| This helps understand the justifications for the change, and
| prevents unnecessary feedback loops to an incredibly high
| degree.
| b3morales wrote:
| In my opinion this is the purpose of a commit message.
| Header: short description of the change; body: details,
| including justification for the change. But I agree, it's
| also good when people preemptively make comments on their own
| commits in the code review interface.
|
| Question for you though: do you have a size limit on this
| requirement? I.e., changes that are small enough don't need
| to do this?
| pojzon wrote:
| This often saves me a lot of time. Not only on finding simple
| issues but also more complex ones.
|
| Self-Review should be advertised more as a clean-code practice
| in the industry.
| dimal wrote:
| 1000x yes. When I finish a significant PR, I almost always wait
| until the next day to put it up. When you've spent 3-4 days
| grinding away at a problem, the urge is to just put it up and
| get rid of it. But when I sleep on it and look at the code from
| someone else's perspective the next day, I always find a ton of
| easy fixes that I didn't see at 5pm the previous day. It makes
| the review so much less painful for everyone.
| UglyToad wrote:
| I think this is about 98% of the value I get from code review,
| just looking at the diff in the GitHub view generally results
| in me spotting all the bugs and stylistic issues that would be
| raised in review.
|
| This makes waiting days for the actual approval all the more
| frustrating!
| onion2k wrote:
| _This makes waiting days for the actual approval all the more
| frustrating!_
|
| In your next stand up raise the fact you're waiting for a
| review as a blocker, and watch as reviews suddenly start
| getting done much more quickly.
| weeksie wrote:
| I was happy to see he mentioned "pair" reviewing. I detest pair
| programming but pair reviewing is excellent. Whenever the time
| and culture of a place permits, I do everything I can to walk
| through my pull requests with one or two people. It's such a high
| value activity, especially when it comes to teams with a good mix
| of junior and senior people.
| rpastuszak wrote:
| > I detest pair programming
|
| why?
| weeksie wrote:
| I don't like it for myself because I don't work well that
| way. I don't like working at companies that use pair
| programming because most of the code I've seen come out of
| pairing sessions is not very good. I realize that's likely to
| start a religious war in certain areas but I've been doing
| this for 20 years and that's been my experience so far.
|
| I think pairing sessions can be great for teaching or for
| getting some needed distance from a problem, but as a mode of
| production I haven't enjoyed my experiences with it.
| BurningFrog wrote:
| One rarely mentioned thing about pair programming is that
| it's a learned skill.
|
| If you put two arbitrary programmers in front of a computer
| and ask them to pair program, you'll probably end up with
| two annoyed people, and not much code.
|
| I was fortunate to join a team of experienced pairers who
| showed how it's done, and after ~3 weeks of ramping up, I
| was fully into it.
| pmcollins wrote:
| My brain is a very bad virtual machine and it's very bad at
| parsing diffs (even GitHub ones), so I like to check out the code
| under review locally, browse it and see what my IDE says about it
| (thank you JetBrains), run it, and run the tests while stepping
| through the code. Makes a big difference.
| indymike wrote:
| One of the best moves our team ever made was moving to code
| review means actually checking out and running the code in our
| IDE. So many little improvements happened: logs got better,
| runtime-messages got better. Since we were reviewing in the
| IDE, running the profiler was a snap, and dropping a breakpoint
| in a diff lising let the reviewer inspect state right at the
| point of change. Another benefit: it was a lot easier to fix
| bugs found in review than just punt to the original
| developer... so the reviews became a lot less painful.
| fishywang wrote:
| I have this script: $ cat ~/bin/git-pr
| #!/bin/sh if [ $# -lt 1 ]; then exit
| -1 fi git fetch origin
| refs/pull/$1/head:pr/$1
|
| So I can run `git pr <number>` to get `pr/<number>` branch
| created locally that I can checkout to.
| nurgasemetey wrote:
| Also there is github cli for checking out PR
| $ gh pr checkout {<number> | <url> | <branch>}
| mook wrote:
| You can add to the set of refs that git pulls (via
| remote.origin.refs); so for me it's `git checkout
| pr/123/head` (or /merge) instead, without creating a local
| branch that you need to delete later.
| hpb42 wrote:
| You can also configure git to fetch the GitHub PR
| branches[0]. Add this line to your .git/config:
| fetch = +refs/pull/*/head:refs/remotes/upstream/pr/*
|
| This has to be done in each repository you have checked out
| locally, but maybe it is possible to also have this done
| globally. But I never tried to do it.
|
| [0] http://tiborsimko.org/github-local-handling-of-pull-
| requests...
| codeflo wrote:
| That's an underappreciated technique, and I'm not sure why.
| Someone once remarked (verbally) in the context of a code
| review that they would need to run the code to fully understand
| it, to which I replied "Why don't you?" The look on their face
| was priceless, first confusion, then enlightenment. There's no
| law or invisible wall that prevents you from leaving that web-
| based diff viewer.
| stumpf wrote:
| A previous coworker taught me to do this :finger-guns: I use
| VSCode with the GitHub pull requests extension now, and it's
| such a better experience than attempting to review directly on
| github.com. My reviews are not just quicker but also higher
| quality since I can easily step through code changes directly
| in the IDE. Great advice.
| Vinnl wrote:
| These are good tips. One more thing, if we're talking GitHub
| specifically: the labels of the statuses you can give your review
| are somewhat misleading, in that their actual effects don't quite
| match the labels. So:
|
| - "Request changes": this isn't so much a request, as it is
| "prevent this person from merging this until I specifically
| review it again". It's fairly harsh.
|
| - "Comment": effectively, "require another review before it can
| be merged, not necessarily by me".
|
| - "Approve": the submitter can merge it at their prerogative.
|
| It is very common for me to hit "approve" even though I've
| requested changes, as they're often
|
| > so trivial that you don't think it needs a second review
| afterwards
|
| or even totally reasonable for someone to push back against or
| not implement.
|
| _Sometimes_ I 'll have seen a non-trivial bug, in which case
| I'll set it to "comment". I practically never use "request
| changes".
| cjcenizal wrote:
| Sometimes for particularly complex changes, I like to comment out
| lines of logic and see if any tests break. It's a fast and easy
| way to identify if the test coverage is adequate. Code coverage
| report generators are a good way to automate this, but not all
| projects use them and sometimes just because logic branches are
| covered by a test doesn't mean the variations in behavior created
| by those branches have been covered. Tests that pass after I've
| deleted some code flushes these problems into the open.
| raziel2p wrote:
| Mutation testing is basically a method of automating this.
___________________________________________________________________
(page generated 2022-05-21 23:01 UTC)