Code review tool recommendations \ stacker news ~tech

2007 sats \ 40 comments \ @k00b 20 Aug 2023 tech

I haven't done much code review my career. (I've mostly been a lone wolf.) So I kind of have fresh eyes and the tooling available is disappointing.

For example, when I'm reviewing I'd like to "tag" parts of a diff for further review and write notes for myself, but neither github nor gitlens give me the ability.

So I'm looking for that Good Good code review tooling. Any recommendations?

view all related items

242 sats \ 2 replies \ @TonyGiorgio 20 Aug 2023

Why tag and not comment directly? If it's truly raw thoughts, keep the review in draft and continue adding comments while you review. You may find the answer by the time you're done.

If still a "need to check these things" kind of comment, be verbal about it. Nobody in dev will expect you to know every single thing about your codebase. You may even get further analysis and answers from the author. At the very least, you are honest about the delay, and are giving an opportunity for the author to speed it up.

10 sats \ 1 reply \ @k00b OP 20 Aug 2023

keep the review in draft

Ah, I haven't played much with draft saving on GH. Maybe that will be enough.

Why tag and not comment directly?

I like to progressively enhance any kind of work that I do, e.g. draft 1, draft 2, draft 3, publish1, publish2, publish3. Most code review tools don't seem to support it (at least not in an obvious way).

Perhaps I'm overthinking, but I also like to minimize RTT when communicating about things.

If still a "need to check these things" kind of comment, be verbal about it. Nobody in dev will expect you to know every single thing about your codebase. You may even get further analysis and answers from the author. At the very least, you are honest about the delay, and are giving an opportunity for the author to speed it up.

Great recs.

10 sats \ 0 replies \ @_redacted 20 Aug 2023

If you set a "Requires approval" + "Require review from Code Owners" in branch protection, then drafts let you collect your thoughts before publishing them and give security/confidence that things won't be merged prematurely. You can also comment (or not) before approving, and with CODEOWNERS can set where you need to be involved vs someone else.

121 sats \ 4 replies \ @WeAreAllSatoshi 20 Aug 2023

I can’t help but feel responsible for this post lol

ETA: You should be able to add comments to the diff under the pending review, and you can choose to remove them before “releasing” the review to the developer.

0 sats \ 3 replies \ @k00b OP 20 Aug 2023

lol you are. It looks really good by the way. Very tasteful and I'm really impressed with how well you navigated all the quirks of our codebase.

I do have enhancement suggestions though. Would you prefer I simply make the changes myself or would you like me to just suggest them? It's about the same amount of work for me either way, but in the latter case you potentially learn more about the codebase/patterns/deps (assuming my suggestions aren't full of crap ... which is plausible).

121 sats \ 2 replies \ @WeAreAllSatoshi 20 Aug 2023

Thank you, I appreciate the kind words!

Let's discuss the suggestions on the PR. That way they're documented for others to review later, more potential learning for me, etc.

0 sats \ 1 reply \ @nemo 20 Aug 2023

deleted by author

0 sats \ 0 replies \ @nemo 20 Aug 2023

deleted by author

100 sats \ 0 replies \ @john_doe 21 Aug 2023

At work I have used Backlog and Gerrit and would say Gerrit was pretty good. While you are doing comments you can apply it or not to save it. People would reviewed my code were just writing it in the comment: "Note to self: etc". The diff was also good with Gerrit, it highlights changes in a better way than others. I don't know if it changed or not though since now I am also a lone wolf using Gitlab.

100 sats \ 24 replies \ @_redacted 20 Aug 2023

With decade+ on github I’d say that while it doesn’t give you private notes, tagging a line/range with an appropriate comment might work. I’d normally leave something like “this makes sense but doesn’t feel right for some reason and I’d like to think more before approving”.

Are you open to a non-GitHub workflow overall or looking for things in addition to GitHub?

100 sats \ 21 replies \ @nemo 20 Aug 2023

deleted by author

10 sats \ 8 replies \ @k00b OP 20 Aug 2023

Hmmm I'm not sure this is what you're suggesting, but maybe I should save the git diffs to a file locally and write all my notes there!

10 sats \ 7 replies \ @nemo 20 Aug 2023

deleted by author

100 sats \ 3 replies \ @nemo 20 Aug 2023

deleted by author

10 sats \ 2 replies \ @nemo 20 Aug 2023

deleted by author

31 sats \ 1 reply \ @k00b OP 20 Aug 2023

Noooo I don't want more tools lol. I want one really good one. 😄

10 sats \ 0 replies \ @nemo 20 Aug 2023

deleted by author

reply on another page

31 sats \ 2 replies \ @k00b OP 20 Aug 2023

In some cases the code is fine but I suspect it could have uncaught corner cases or improved but I'm not 100% sure and I don't want to "alert" the author if I'm wrong.

100 sats \ 0 replies \ @_redacted 20 Aug 2023

Corner cases are were code reviews are really helpful! It's taken many years to separate "you should have done it how I would have done it" from "this code is ok to be merged". It's helpful to think "does this pr improve the code overall".

Bugs will happen. Keeping the critical sats-related code separate from the rest, + CODEOWNERS will keep the critical sections safer while letting the rest change more quickly.

10 sats \ 0 replies \ @nemo 20 Aug 2023

deleted by author

0 sats \ 11 replies \ @_redacted 20 Aug 2023

How does tig compare to blame view it github? What makes you pick that?

I love love love blame when trying to figure out wtf happened with code. Hampered mostly by ... could-be-improved commit messages / history.

0 sats \ 10 replies \ @nemo 20 Aug 2023

deleted by author

0 sats \ 0 replies \ @_redacted 20 Aug 2023

Oh, that looks similar to git log --graph more

0 sats \ 8 replies \ @_redacted 20 Aug 2023

Are you looking at the history for your local history or do you pull down the PR ref and look through there?

0 sats \ 7 replies \ @nemo 20 Aug 2023

deleted by author

0 sats \ 6 replies \ @_redacted 20 Aug 2023

Are you collaborating on a branch/pr? All pushing to a mainline branch?

(asking as I normally do cicd automation and like knowing more about how folks work)

21 sats \ 2 replies \ @nemo 20 Aug 2023

deleted by author

view replies

0 sats \ 2 replies \ @nemo 20 Aug 2023

deleted by author

view replies

0 sats \ 1 reply \ @k00b OP 20 Aug 2023

I'm open to anything tbh. We are getting a lot more open source contributions, and if I can review better I can respect the work more.

100 sats \ 0 replies \ @_redacted 20 Aug 2023

Overall having comments in a not-github way has a noticeable increase in difficulty for the general-coding public.

It's worth trying to do things more openly/in-github when possible (IMO). Github is quite opinionated though and does take time to adapt workflow to the github-way.

There are a bunch of branch protection rules that can be combined depending on how much you want to block merging before the review is "finished".

100 sats \ 1 reply \ @Joe 26 Aug 2023

Review Board is a web-based, open source tool for code review. To test this code review tool, you can either explore the demo on their website or download and set up the software on your server

The Python programming language and its installers, MySQL or PostgreSQL as a database, and a web server are the prerequisites to run Review Board on a server.

You can integrate Review Board with a wide range of version control systems — Git, Mercurial, CVS, Subversion and Perforce. You can also link Review Board to Amazon S3 for storing screenshots directly in the tool.

0 sats \ 0 replies \ @k00b OP 26 Aug 2023

We used review board at my old job. I for some reason thought it only supported subversion.