9 ms·

Commenting and approving pull requests

90 points by jwworth 6mo ago

deleted 5mo ago
[deleted]
shevy-java 5mo ago
I approve of it. I have now commented on it.
quxbar 5mo ago
I started doing this a few years ago, it works a treat with non-junior engineers. With the right testing in place, review becomes more about communicating the ideas clearly rather than gatekeeping.
singron 5mo ago
I'm a big believer in this workflow and generally agree with all the guidelines about when to do and not do this. Code review has value, but I don't think we are always honest about the costs. At most places I've worked, there has been an informal understanding that code should be reviewed within 24 hours or so, but there is a world of difference between getting a review within 2 hours and 23 hours. If you have to go back and forth a second time, it's so much more likely that the approval comes much later due to straddling end-of-day in someone's timezone. Tangentially, if you are designing policies for code review at your org, try to think both about minimum requirements (e.g. PRs should get a first look within 24 hours) and typical requirements (e.g. most PRs should get reviewed within 2-3 hours). What typically happens is what determines overall velocity for your org, so it's much more important than whatever strict SLA policy you have. These are also fixable problems if you have targeted conversations with people. E.g. "I noticed X, Y, Z times, you had unreviewed PRs at the end of the day. Can you set aside some time to review code before EOD? Or try installing PR reminder?"
eterm 5mo ago
Azure-dev ops explicitly has an "Approve with suggestions", I suspect it was designed for when there are multiple acceptances required, but I sometimes use it anyway. Whether those comments get read once approved, I don't know.
embedding-shape 5mo ago
> I almost always leave a comment on each PR I review, even just observations: “This class is getting big, we might want to consider adding a presenter,” or praise: “Thanks for cleaning this up!” Things like that I'd much rather leave as comments in the code, rather than dangling as off-hand things in some unrelated PR. Probably no one will see those PR comments unless they go splunking for some reason, but having them right in the code is much more evident to all the people passing through, or even reminding yourself in the future too. In general I feel like PR reviews are basically reviews happening too late, and if there is a lot of stuff to review and agree upon in the PR, you'll be better off trying to reduce that up front by discussing and noting design decisions before the work even starts. If there is so many unknowns first, do one step to figure out how it can be done, then agree on that, again before starting the actual work. That leads to PRs basically just being spot-checking, not deep analysis nor even space for nitpicks. If you as a reviewer spots things after the implementation rather in the discussion beforehand, that ends up being on both of you, instead of just the implementer who tried to move along to finish the thing.
- Someone1234 5mo ago
  I'm not following this. PRs are the first time your reviewers have seen that change, so there is no opportunity downstream to do the things you're suggesting. You're essentially suggesting pre-PRs, but it is circular, since those same pre-PRs would have the same criticism. PRs are about isolated core changes, not feature or system design. They answer how not why.
  - bulbar 5mo ago
    > You're essentially suggesting pre-PRs, but it us circular, since those same pre-PRs would have the same criticism. Walking this road to the end you get pair programming.
    - esafak 5mo ago
      You get to design committees where everything has to be approved in advance.
Groxx 5mo ago
This is more than half of my code reviews (when functionally correct). It works pretty well, when you can trust that the person doing the changes won't accidentally (or otherwise) slip in behavioral changes. Like half just got merged as-is and half of those got a separate cleanup later. Most people are fine, some routinely abuse it to do much larger changes that should normally imply full review - double check afterward! Your name is still on it, it is still your responsibility.
adamgordonbell 5mo ago
I like doing this as well. The 'auto merge on approval flag' PR authors can flip on GitHub breaks this flow though, as it will just merge as soon as you hit approve.
- yojo 5mo ago
  I also follow this approach. I just flip the flag on the PR I’m reviewing to off before submitting my approval. We also have most of our repos set to block if unresolved comments. I think it’s a flag on branch protection rules
  - namenotrequired 5mo ago
    The way my company works, it doesn’t break it. You approve and merge, then any suggestions can be implemented in a next PR That way the average customer doesn’t need to wait for your code style change or edge case fix
65 5mo ago
I do this too. I generally skim PRs just to make sure the person isn't doing anything too crazy. I trust my team to write good code. Pulling a branch and testing the code is usually a waste of time unless it's a critical bug or feature.
jez 5mo ago
> Additionally, some repos can be configured to automatically merge PRs when all requirements are met, one of which might be your approval. If anyone at GitHub is reading this, I’d love a fourth checkbox in the “leave a review” modal that is “Approve but disable auto merge” (alongside Comment/Approve/Request changes)! Even just surfacing “this PR has auto merge enabled” near the Approve button would be great.
- yojo 5mo ago
  You might try adding this branch protection rule to require conversation resolution: https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/about-protected-branches#require-conversation-resolution-before-merging https://docs.github.com/en/repositories/configuring-branches...
  - theowaway213456 5mo ago
    This feels like a suboptimal solution to me because I personally like to keep comments in "unresolved" state so that they remain visible and other folks can weigh in on them if they want, but in a way that doesn't block the PR. Basically I wish that GitHub would either separate the "collapsed" and "resolved" concepts, or add this "approve without merging" button.
gedge 5mo ago
This one resonates, because it's largely how I comment on PRs too! One thing not mentioned is how important it is to acknowledge the comments too. People are taking their time to review your PR, and not even giving a reaction will make the commenter question whether or not it was even received. I'm not looking to throw my thoughts out into the aether. That's what microblogging platforms are for! I can't tell you how many times I got no response/acknowledgement on a comment that actually surfaced something critical. I haven't been keeping track, but I think my comments could have prevented dozens of outages at this point. It's quite exhausting. In my own experience, the worst offenders of this are senior devs. > Why approve, if I’ve left comments that I think are worth implementing? > > Because I trust my team. I know that my comments will be considered, and if they’re useful, implemented. I do this a lot too. It's critical that PR authors don't burn that trust either. If they make substantial changes that warrant another review, I hope they do request it. Too many times in my career have colleagues just went ahead and made bad changes after my approval that I would have easily caught, merge, and things go High trust, high alignment environments move so fast, and you know when you're in one and know when you have your colleagues' trust. It feels really good!
- jt2190 5mo ago
  Isn’t a change to the code that incorporates your suggestions enough acknowledgment? Presumably the code change would be required to get your approval?
  - gedge 5mo ago
    Of course! I didn't state it explicitly, but what I was referring to were comments that went completely unacknowledged in any way, be it explicit or implicit. No changes. No reaction. No "resolve comment". Radio silence. As mentioned, I've experienced it too many times where not addressing a question/concern I put on the pull request led to outages that could have been avoided. I think it's typically a certain personality. It's not a common occurrence, but I have experienced it.
4lx87 5mo ago
I woke up one day and the industry is gating all code changes behind PRs w/ approvals. Anyone still able to commit to main without approval from someone else? I’m even seeing companies require multiple approvals on every PR. It’s insanity. I’m skeptical. Software is as buggy as it ever was. I come across teams shipping terrible quality software, where every line change was approved and reviewed. I come across teams that require every change have an approval, but don’t require 100% test coverage. I’m seeing senior engineers have to get approval from juniors for a copy change. It’s theatre. It’s bad management. It’s a cargo cult. It isn’t actually driving code quality. Code review is one thing. Code review is good. But requiring every change have an approval is something else.
- andrewjf 5mo ago
  There's two kinds of reviews in my experience: 1. Does it work? Then ship it. This is great for early on, high-velocity where the goal is to get something working in the wild. AI and AI proponents love this option. It's easy to spot obvious problems, but very unlikely to lead to feedback on structural changes to abstractions and architecture to increase overall _long-term_ velocity. 2. We assume this works, but is it "correct"? This is where long-term code maintainability is created. The quality and effort put into a review like this is obviously far more involved than option 1. People working long term on a code base love this option. We've been biased towards #1 for a long time, but I feel like we dont have enough people capable of doing #2.
  - motoroco 5mo ago
    I've worked with some people who only seem to care about 2. as in, they don't try the feature in any way, but come back with comments about "this isn't tested enough" even though it has higher coverage than the codebase's average, and refuse to approve even though they'll never meaningfully review the content. it does seem to be mostly just theater in my experience
- Pannoniae 5mo ago
  exactly, the kind of reviews which have a point are the ones you do before starting work. But that's a design review, not a code review. My team does "commit to master", although I did catch a fair few regressions by looking at the committed code.... but as long as it doesn't go live, who cares?
scastiel 5mo ago
Totally agree, and I go even farther: I consider that the PR author is totally responsible of merging their PR or not, and so no comment is blocking. I can make a comment I consider blocking, but there might be very good reasons that I can't see to still merge the PR. Fixes can come in a follow-up PR. As long as everyone takes responsibility for their work, it is a well-working system.
punkbit 5mo ago
I do the same! But PR discussions about lintable style issues always surprise me. The ideal solution is to add a rule in the linter. But when the team won't agree on the rule, and is open to multiple styles, the author should decide, simple! Had a team mate recently who'd block PRs over T[] vs Array<T>, forcing people to stick with Array<T> for simple types like number[] even though TypeScript's own docs and tooling push T[].
- Supermancho 5mo ago
  Same. I've said my peace, but I don't let that get in the way of what works. Notably, prevent github merges if there are unresolved comments, so you know they glanced over them before collapsing it.
- epgui 5mo ago
  Linters often don’t provide the constraints you desire. While they’re a great (arguably essential) tool, they often are not sufficient. When a project has well-established patterns, part of the job is to just follow the pattern, whether you like it or not, until you find a reason to 1) change the pattern everywhere, or; 2) make an exception to the pattern with intention.
  - __alexs 5mo ago
    Write a lint rule. It's often quite easy.
    - epgui 5mo ago
      What linter are you assuming I’m running? You can’t always write your own rules.
  - punkbit 5mo ago
    It's quite simple to write a rule, e.g. for the example I shared https://typescript-eslint.io/rules/array-type https://typescript-eslint.io/rules/array-type. But I've never seen any reputable project arguing against T[] for simple types. In my opinion, that says a lot about the reviewer, not the contributor. If a linter rule doesn't exist, you should argue against the idiomatic convention.
Quarrelsome 5mo ago
While one of the best things about PRs can be raising the floor on quality, I think the another powerful factor is forcing an opportunity to talk about the code. This article even pre-empts this confusion by acknowledging that commenting on a PR while approving it is "weird". This implies that many people don't understand the indirect benefits of the PR and think the process is all about approval and raising the quality floor. I think raising the floor, gives diminishing returns once someone is used to the team and the code base, but the conversation always remains relevant. Sometimes teams that resist things like PRs (e.g. "they just slow us down") are actually teams that are having those conversions elsewhere (in-person, on slack, during standup or sprint planning, etc).
- TheGRS 5mo ago
  Totally agreed, but it always comes back to managing the average over the best. It is a lot more effective to enforce a culture of approvals than a culture of collaboration. Managers should be striving for the later, but not every team is built the same way and many in leadership just need assurance that the bare minimum is getting done.
redoh 5mo ago
[flagged]
TheGRS 5mo ago
This is me, I think it comes from a personality of wanting to make sure I say something but generally trusting that the author has more context than I do. I float questions and nits but generally will approve if I don't see anything glaring. If I spot anything where I think "we should definitely go another direction here" I ask for changes instead of approving and make them super clear.
philipnee 5mo ago
Just want to comment here. I hate reviewer leave a bunch of nits and stamp the PR. This is ambiguous, are these nits asks or just you opinions? What if I dont address all of them. Also folks need to take rejection lightly - your reviewer wants you to address something, thats really it.
- lexicality 5mo ago
  Hi, I do this. > are these nits asks or just you opinions? If I've approved the PR, then these are changes I'm asking you to do, but not ordering you to do. You are free to say "no" to my request > What if I dont address all of them Then you will have decided that you don't agree with my recommendation and that's OK. I only ever do this with people I trust - I am trusting you to review each of my nitpicks and make an informed decision if they're necessary or not. Generally I'd like you to reply to the ones you don't do with a reason though.
  - Linux-Fan 5mo ago
    On Github, there is a way to leave individual comments in the code and in addition give a review a summary. In addition to hitting the "approve" button, I typically spell it out explicitly in this summary: "Please check my comments and see if anything makes sense to implement." Often, I also take this opportunity to point out the "one" most valuable change in my opinion. If the developer of the code doesn't find any of the comments to be applicable/usefuly, they can always go ahead and merge it right away.
- deleted 5mo ago
  [deleted]
- deleted 5mo ago
  [deleted]
- Terr_ 5mo ago
  I try to tag the line-by-line comments with little labels like [Unimportant] or [Style] so that someone going through them has an idea of their (un)importance without reading the whole thing.
musicmatze 5mo ago
I see where the author is coming from, but still this feels like a reinvention of commit trailers ("Acked-by", "Reviewed-by",...) especially for these non-blocking and "only appraising" comments. Commit trailers even have the benefit that they are recorded in the comment, making them discoverable and even allowing statistics, while "human language" comments are not that easy to do statistics on (modulo AI I guess, but that's another topic).
dbalatero 5mo ago
I do the same as the author, and for my comments I make it clear what is non-blocking/blocking with Conventional Comments: https://conventionalcomments.org https://conventionalcomments.org To make it easier for myself to leave the CC tags ("nit(non-blocking): ", etc), I use the macOS text expander in System Settings and created mnemonics to easily insert them. Example: If I type "+pnn", that maps to: +p = pull request comment n = nit n = non-blocking Example 2: If I type "+pc", that maps to: +p = pull request comment c = chore (and it's blocking because I didn't type "+pcn", the non-blocking version).
phyzix5761 5mo ago
With a robust enough test suite and a team that does TDD and mob programming, code reviews are pretty much obsolete and a waste of time. Everyone's already involved in the coding process as a mob and the tests catch any regressions.
- jjmarr 5mo ago
  There's two purposes to review. One is to enforce ownership, and the other is to provide mentorship. Even if you don't have ownership, mentoring is still useful. I get asked for review by teammembers on an area I have expertise in. Their solution might work but cause problems later. With review, I can knowledge transfer my lived experience so they don't suffer like I did. The third purpose to review is stylistic nitpicking and formatting as a simulacrum of actual work. This is useless and turns people off from review.
  - phyzix5761 5mo ago
    Yes, I agree, but when the whole team is already part of the development process through mob programming there's no one who hasn't seen the code, hasn't provided feedback/mentorship, and hasn't voiced their stylistic concerns by the time the development is done.
    - jjmarr 5mo ago
      If your team is 5 people then sure, it's fine. From my experience in a massive shared repo with unclear ownership...
- rsalus 5mo ago
  100p agree. Most code reviews devolve to nitpicking anyway. I think it's much more valuable for teammates to review the _design_ and _intent_ rather than the explicit artifacts.
christophilus 5mo ago
Does anyone not do this? It would be miserable working at a place where nits were blockers and required re-review.
namenotrequired 5mo ago
We do this too. In my team, my rule is: if it’s better than what’s on master, you approve and merge. There’s no use making the customer wait for your questions, code style suggestions etc to be addressed. Even if you request changes, you leave all your comments and make explicit which are the blocking ones and which can be addressed in the future.
- allendoerfer 5mo ago
  > In my team, my rule is: if it’s better than what’s on master, you approve and merge. This causes unnecessary code changes later on, code changes mean new code, new code has bugs. The team should try to get it close to perfect on the first try instead. They won't, but that should be everyone's target. If that sounds impossible, then the PR was to big.
  - namenotrequired 5mo ago
    New code has bugs regardless of whether it entered on the same PR or a new one
deleted 5mo ago
[deleted]
paulfharrison 5mo ago
This advice can also be applied to PhD thesis examinations and paper reviews.
tecoholic 5mo ago
This is one of those articles that I read and realise how different people and process are. I fully agree with this method. In fact, my surprise mostly stems from the necessity to write this article. So there are places where comments on PRs are avoided as they are considered “unapproved”? Man oh man.
kuahyeow 5mo ago
Just use blocking/non-blocking. The issue with nitpicks: different reviewers have different definitions of what a "nit" is.

90 points by jwworth 6mo ago

deleted 5mo ago

[deleted]

shevy-java 5mo ago

I approve of it. I have now commented on it.

quxbar 5mo ago

I started doing this a few years ago, it works a treat with non-junior engineers. With the right testing in place, review becomes more about communicating the ideas clearly rather than gatekeeping.

singron 5mo ago

I'm a big believer in this workflow and generally agree with all the guidelines about when to do and not do this. Code review has value, but I don't think we are always honest about the costs. At most places I've worked, there has been an informal understanding that code should be reviewed within 24 hours or so, but there is a world of difference between getting a review within 2 hours and 23 hours. If you have to go back and forth a second time, it's so much more likely that the approval comes much later due to straddling end-of-day in someone's timezone. Tangentially, if you are designing policies for code review at your org, try to think both about minimum requirements (e.g. PRs should get a first look within 24 hours) and typical requirements (e.g. most PRs should get reviewed within 2-3 hours). What typically happens is what determines overall velocity for your org, so it's much more important than whatever strict SLA policy you have. These are also fixable problems if you have targeted conversations with people. E.g. "I noticed X, Y, Z times, you had unreviewed PRs at the end of the day. Can you set aside some time to review code before EOD? Or try installing PR reminder?"

eterm 5mo ago

Azure-dev ops explicitly has an "Approve with suggestions", I suspect it was designed for when there are multiple acceptances required, but I sometimes use it anyway. Whether those comments get read once approved, I don't know.

embedding-shape 5mo ago

> I almost always leave a comment on each PR I review, even just observations: “This class is getting big, we might want to consider adding a presenter,” or praise: “Thanks for cleaning this up!” Things like that I'd much rather leave as comments in the code, rather than dangling as off-hand things in some unrelated PR. Probably no one will see those PR comments unless they go splunking for some reason, but having them right in the code is much more evident to all the people passing through, or even reminding yourself in the future too. In general I feel like PR reviews are basically reviews happening too late, and if there is a lot of stuff to review and agree upon in the PR, you'll be better off trying to reduce that up front by discussing and noting design decisions before the work even starts. If there is so many unknowns first, do one step to figure out how it can be done, then agree on that, again before starting the actual work. That leads to PRs basically just being spot-checking, not deep analysis nor even space for nitpicks. If you as a reviewer spots things after the implementation rather in the discussion beforehand, that ends up being on both of you, instead of just the implementer who tried to move along to finish the thing.

Someone1234 5mo ago
I'm not following this. PRs are the first time your reviewers have seen that change, so there is no opportunity downstream to do the things you're suggesting. You're essentially suggesting pre-PRs, but it is circular, since those same pre-PRs would have the same criticism. PRs are about isolated core changes, not feature or system design. They answer how not why.
- bulbar 5mo ago
  > You're essentially suggesting pre-PRs, but it us circular, since those same pre-PRs would have the same criticism. Walking this road to the end you get pair programming.
  - esafak 5mo ago
    You get to design committees where everything has to be approved in advance.

Groxx 5mo ago

This is more than half of my code reviews (when functionally correct). It works pretty well, when you can trust that the person doing the changes won't accidentally (or otherwise) slip in behavioral changes. Like half just got merged as-is and half of those got a separate cleanup later. Most people are fine, some routinely abuse it to do much larger changes that should normally imply full review - double check afterward! Your name is still on it, it is still your responsibility.

adamgordonbell 5mo ago

I like doing this as well. The 'auto merge on approval flag' PR authors can flip on GitHub breaks this flow though, as it will just merge as soon as you hit approve.

yojo 5mo ago
I also follow this approach. I just flip the flag on the PR I’m reviewing to off before submitting my approval. We also have most of our repos set to block if unresolved comments. I think it’s a flag on branch protection rules
- namenotrequired 5mo ago
  The way my company works, it doesn’t break it. You approve and merge, then any suggestions can be implemented in a next PR That way the average customer doesn’t need to wait for your code style change or edge case fix

65 5mo ago

I do this too. I generally skim PRs just to make sure the person isn't doing anything too crazy. I trust my team to write good code. Pulling a branch and testing the code is usually a waste of time unless it's a critical bug or feature.

jez 5mo ago

> Additionally, some repos can be configured to automatically merge PRs when all requirements are met, one of which might be your approval. If anyone at GitHub is reading this, I’d love a fourth checkbox in the “leave a review” modal that is “Approve but disable auto merge” (alongside Comment/Approve/Request changes)! Even just surfacing “this PR has auto merge enabled” near the Approve button would be great.

yojo 5mo ago
You might try adding this branch protection rule to require conversation resolution: https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/about-protected-branches#require-conversation-resolution-before-merging https://docs.github.com/en/repositories/configuring-branches...
- theowaway213456 5mo ago
  This feels like a suboptimal solution to me because I personally like to keep comments in "unresolved" state so that they remain visible and other folks can weigh in on them if they want, but in a way that doesn't block the PR. Basically I wish that GitHub would either separate the "collapsed" and "resolved" concepts, or add this "approve without merging" button.

gedge 5mo ago

This one resonates, because it's largely how I comment on PRs too! One thing not mentioned is how important it is to acknowledge the comments too. People are taking their time to review your PR, and not even giving a reaction will make the commenter question whether or not it was even received. I'm not looking to throw my thoughts out into the aether. That's what microblogging platforms are for! I can't tell you how many times I got no response/acknowledgement on a comment that actually surfaced something critical. I haven't been keeping track, but I think my comments could have prevented dozens of outages at this point. It's quite exhausting. In my own experience, the worst offenders of this are senior devs. > Why approve, if I’ve left comments that I think are worth implementing? > > Because I trust my team. I know that my comments will be considered, and if they’re useful, implemented. I do this a lot too. It's critical that PR authors don't burn that trust either. If they make substantial changes that warrant another review, I hope they do request it. Too many times in my career have colleagues just went ahead and made bad changes after my approval that I would have easily caught, merge, and things go High trust, high alignment environments move so fast, and you know when you're in one and know when you have your colleagues' trust. It feels really good!

jt2190 5mo ago
Isn’t a change to the code that incorporates your suggestions enough acknowledgment? Presumably the code change would be required to get your approval?
- gedge 5mo ago
  Of course! I didn't state it explicitly, but what I was referring to were comments that went completely unacknowledged in any way, be it explicit or implicit. No changes. No reaction. No "resolve comment". Radio silence. As mentioned, I've experienced it too many times where not addressing a question/concern I put on the pull request led to outages that could have been avoided. I think it's typically a certain personality. It's not a common occurrence, but I have experienced it.

4lx87 5mo ago

I woke up one day and the industry is gating all code changes behind PRs w/ approvals. Anyone still able to commit to main without approval from someone else? I’m even seeing companies require multiple approvals on every PR. It’s insanity. I’m skeptical. Software is as buggy as it ever was. I come across teams shipping terrible quality software, where every line change was approved and reviewed. I come across teams that require every change have an approval, but don’t require 100% test coverage. I’m seeing senior engineers have to get approval from juniors for a copy change. It’s theatre. It’s bad management. It’s a cargo cult. It isn’t actually driving code quality. Code review is one thing. Code review is good. But requiring every change have an approval is something else.

andrewjf 5mo ago
There's two kinds of reviews in my experience: 1. Does it work? Then ship it. This is great for early on, high-velocity where the goal is to get something working in the wild. AI and AI proponents love this option. It's easy to spot obvious problems, but very unlikely to lead to feedback on structural changes to abstractions and architecture to increase overall _long-term_ velocity. 2. We assume this works, but is it "correct"? This is where long-term code maintainability is created. The quality and effort put into a review like this is obviously far more involved than option 1. People working long term on a code base love this option. We've been biased towards #1 for a long time, but I feel like we dont have enough people capable of doing #2.
- motoroco 5mo ago
  I've worked with some people who only seem to care about 2. as in, they don't try the feature in any way, but come back with comments about "this isn't tested enough" even though it has higher coverage than the codebase's average, and refuse to approve even though they'll never meaningfully review the content. it does seem to be mostly just theater in my experience
Pannoniae 5mo ago
exactly, the kind of reviews which have a point are the ones you do before starting work. But that's a design review, not a code review. My team does "commit to master", although I did catch a fair few regressions by looking at the committed code.... but as long as it doesn't go live, who cares?

scastiel 5mo ago

Totally agree, and I go even farther: I consider that the PR author is totally responsible of merging their PR or not, and so no comment is blocking. I can make a comment I consider blocking, but there might be very good reasons that I can't see to still merge the PR. Fixes can come in a follow-up PR. As long as everyone takes responsibility for their work, it is a well-working system.

punkbit 5mo ago

I do the same! But PR discussions about lintable style issues always surprise me. The ideal solution is to add a rule in the linter. But when the team won't agree on the rule, and is open to multiple styles, the author should decide, simple! Had a team mate recently who'd block PRs over T[] vs Array<T>, forcing people to stick with Array<T> for simple types like number[] even though TypeScript's own docs and tooling push T[].

Supermancho 5mo ago
Same. I've said my peace, but I don't let that get in the way of what works. Notably, prevent github merges if there are unresolved comments, so you know they glanced over them before collapsing it.
epgui 5mo ago
Linters often don’t provide the constraints you desire. While they’re a great (arguably essential) tool, they often are not sufficient. When a project has well-established patterns, part of the job is to just follow the pattern, whether you like it or not, until you find a reason to 1) change the pattern everywhere, or; 2) make an exception to the pattern with intention.
- __alexs 5mo ago
  Write a lint rule. It's often quite easy.
  - epgui 5mo ago
    What linter are you assuming I’m running? You can’t always write your own rules.
- punkbit 5mo ago
  It's quite simple to write a rule, e.g. for the example I shared https://typescript-eslint.io/rules/array-type https://typescript-eslint.io/rules/array-type. But I've never seen any reputable project arguing against T[] for simple types. In my opinion, that says a lot about the reviewer, not the contributor. If a linter rule doesn't exist, you should argue against the idiomatic convention.

Quarrelsome 5mo ago

While one of the best things about PRs can be raising the floor on quality, I think the another powerful factor is forcing an opportunity to talk about the code. This article even pre-empts this confusion by acknowledging that commenting on a PR while approving it is "weird". This implies that many people don't understand the indirect benefits of the PR and think the process is all about approval and raising the quality floor. I think raising the floor, gives diminishing returns once someone is used to the team and the code base, but the conversation always remains relevant. Sometimes teams that resist things like PRs (e.g. "they just slow us down") are actually teams that are having those conversions elsewhere (in-person, on slack, during standup or sprint planning, etc).

TheGRS 5mo ago
Totally agreed, but it always comes back to managing the average over the best. It is a lot more effective to enforce a culture of approvals than a culture of collaboration. Managers should be striving for the later, but not every team is built the same way and many in leadership just need assurance that the bare minimum is getting done.

redoh 5mo ago

[flagged]

TheGRS 5mo ago

This is me, I think it comes from a personality of wanting to make sure I say something but generally trusting that the author has more context than I do. I float questions and nits but generally will approve if I don't see anything glaring. If I spot anything where I think "we should definitely go another direction here" I ask for changes instead of approving and make them super clear.

philipnee 5mo ago

Just want to comment here. I hate reviewer leave a bunch of nits and stamp the PR. This is ambiguous, are these nits asks or just you opinions? What if I dont address all of them. Also folks need to take rejection lightly - your reviewer wants you to address something, thats really it.

lexicality 5mo ago
Hi, I do this. > are these nits asks or just you opinions? If I've approved the PR, then these are changes I'm asking you to do, but not ordering you to do. You are free to say "no" to my request > What if I dont address all of them Then you will have decided that you don't agree with my recommendation and that's OK. I only ever do this with people I trust - I am trusting you to review each of my nitpicks and make an informed decision if they're necessary or not. Generally I'd like you to reply to the ones you don't do with a reason though.
- Linux-Fan 5mo ago
  On Github, there is a way to leave individual comments in the code and in addition give a review a summary. In addition to hitting the "approve" button, I typically spell it out explicitly in this summary: "Please check my comments and see if anything makes sense to implement." Often, I also take this opportunity to point out the "one" most valuable change in my opinion. If the developer of the code doesn't find any of the comments to be applicable/usefuly, they can always go ahead and merge it right away.
deleted 5mo ago
[deleted]
deleted 5mo ago
[deleted]
Terr_ 5mo ago
I try to tag the line-by-line comments with little labels like [Unimportant] or [Style] so that someone going through them has an idea of their (un)importance without reading the whole thing.

musicmatze 5mo ago

I see where the author is coming from, but still this feels like a reinvention of commit trailers ("Acked-by", "Reviewed-by",...) especially for these non-blocking and "only appraising" comments. Commit trailers even have the benefit that they are recorded in the comment, making them discoverable and even allowing statistics, while "human language" comments are not that easy to do statistics on (modulo AI I guess, but that's another topic).

dbalatero 5mo ago

I do the same as the author, and for my comments I make it clear what is non-blocking/blocking with Conventional Comments: https://conventionalcomments.org https://conventionalcomments.org To make it easier for myself to leave the CC tags ("nit(non-blocking): ", etc), I use the macOS text expander in System Settings and created mnemonics to easily insert them. Example: If I type "+pnn", that maps to: +p = pull request comment n = nit n = non-blocking Example 2: If I type "+pc", that maps to: +p = pull request comment c = chore (and it's blocking because I didn't type "+pcn", the non-blocking version).

phyzix5761 5mo ago

With a robust enough test suite and a team that does TDD and mob programming, code reviews are pretty much obsolete and a waste of time. Everyone's already involved in the coding process as a mob and the tests catch any regressions.

jjmarr 5mo ago
There's two purposes to review. One is to enforce ownership, and the other is to provide mentorship. Even if you don't have ownership, mentoring is still useful. I get asked for review by teammembers on an area I have expertise in. Their solution might work but cause problems later. With review, I can knowledge transfer my lived experience so they don't suffer like I did. The third purpose to review is stylistic nitpicking and formatting as a simulacrum of actual work. This is useless and turns people off from review.
- phyzix5761 5mo ago
  Yes, I agree, but when the whole team is already part of the development process through mob programming there's no one who hasn't seen the code, hasn't provided feedback/mentorship, and hasn't voiced their stylistic concerns by the time the development is done.
  - jjmarr 5mo ago
    If your team is 5 people then sure, it's fine. From my experience in a massive shared repo with unclear ownership...
rsalus 5mo ago
100p agree. Most code reviews devolve to nitpicking anyway. I think it's much more valuable for teammates to review the _design_ and _intent_ rather than the explicit artifacts.

christophilus 5mo ago

Does anyone not do this? It would be miserable working at a place where nits were blockers and required re-review.

namenotrequired 5mo ago

We do this too. In my team, my rule is: if it’s better than what’s on master, you approve and merge. There’s no use making the customer wait for your questions, code style suggestions etc to be addressed. Even if you request changes, you leave all your comments and make explicit which are the blocking ones and which can be addressed in the future.

allendoerfer 5mo ago
> In my team, my rule is: if it’s better than what’s on master, you approve and merge. This causes unnecessary code changes later on, code changes mean new code, new code has bugs. The team should try to get it close to perfect on the first try instead. They won't, but that should be everyone's target. If that sounds impossible, then the PR was to big.
- namenotrequired 5mo ago
  New code has bugs regardless of whether it entered on the same PR or a new one

deleted 5mo ago

[deleted]

paulfharrison 5mo ago

This advice can also be applied to PhD thesis examinations and paper reviews.

tecoholic 5mo ago

This is one of those articles that I read and realise how different people and process are. I fully agree with this method. In fact, my surprise mostly stems from the necessity to write this article. So there are places where comments on PRs are avoided as they are considered “unapproved”? Man oh man.

kuahyeow 5mo ago

Just use blocking/non-blocking. The issue with nitpicks: different reviewers have different definitions of what a "nit" is.