3 ms·

> 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 pra

> 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 6mo 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 6mo 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 6mo ago
    You get to design committees where everything has to be approved in advance.
    - Someone1234 6mo ago
      Yep, where productivity goes to die and your developers feel no autonomy/trust.
- ljm 6mo ago
  Usually by the time a PR has been submitted it's too late to dig into aspects of the change that come from a poor understanding of the task at hand without throwing out the PR and creating rework. So it's helpful to shift left on that and discuss how you intend to approach the solution. Especially for people who are new to the codebase or unfamiliar with the language and, thanks to AI, show little interest in learning. Obviously not for every situation, but time can be saved by talking something through before YOLOing a bad PR.
  - nightpool 6mo ago
    Yes, it should be cheap to throw out any individual PR and rewrite it from scratch. Your first draft of a problem is almost never the one you want to submit anyway. The actual writing of the code should never be the most complicated step in any individual PR. It should always be the time spent thinking about the problem and the solution space. Sometimes you can do a lot of that work before the ticket, if you're very familiar with the codebase and the problem space, but for most novel problems, you're going to need to have your hands on the problem itself to get your most productive understanding of them. I'm not saying it's not important to discuss how you intend to approach the solution ahead of time, but I am saying a lot about any non-trivial problem you're solving can only be discovered by attempting to solve it. Put another way: the best code I write is always my second draft at any given ticket. More micromanaging of your team's tickets and plans is not going to save you from team members who "show little interest in learning". The fact that your team is "YOLOing a bad PR" is the fundamental culture issue, and that's not one you can solve by adding more process.
    - n_e 6mo ago
      I'm not sure what approach you're suggesting? Asking a more junior developer or someone who "show little interest in learning" to discuss their approach with you before they've spent too much time on the problem, especially if you expect them to take the wrong approach seems like the right way to do things. Throwing out a PR of someone who doesn't expect it would be quite unpleasant, especially coming from someone more senior.
- embedding-shape 6mo ago
  > PRs are the first time your reviewers have seen that change, so there is no opportunity downstream to do the things you're suggesting. Yes, but I'm arguing for that it shouldn't be the first time they hear about that this change is planned and happening, and their input should have taken into account before the PR is even opened, either upfront/before or early in development. This eliminates so many of the typical PR reviews/comments. Figure out where you are going before you start going there, instead of trying to course correct after people already walked places.
swiftcoder 6mo ago
> 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 This is accurate, but it's still an important check in the communication loop. It's not all that uncommon for two engineers to discuss a problem, and leave the discussion with completely different mental models of the solution.
bulbar 6mo ago
> 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. I agree in general and tried to push for a more iterate approach in our team. However, my fear is that this would multiply the effort because it's very easy to discuss many things that do not matter or turn out later to not matter. It seems it's easier for people to only discuss the important stuff when presented as an actual implementation. We are talking tendencies here, of course, general design decision must always be done beforehand.
- embedding-shape 6mo ago
  > It seems it's easier for people to only discuss the important stuff when presented as an actual implementation. LLMs help a lot here, create two prototypes with both designs, compare them together :) Could even evaluate how ergonomic some future potential change is, to see how it holds up in practice. I used to use pen and paper to essentially do the same, minus having real code and instead just talking concepts, but it does suffer from the issue that some need to be confronted with code in front of them to visualize things better for themselves.
Bridged7756 6mo ago
I think it's a fine line to walk. At my job what we do is discuss any complex implementation or risky change or blockers in the dev eng meeting. For smaller stuff, or more straightforward solutions, we don't bring it up. If you make it a hard rule to first discuss all tickets, it just seems draconian. Code review is specifically for code quality, more lower level stuff.
christofosho 6mo ago
You might be approaching PR comments differently than I've seen. When a comment is something to be addressed, it's either put into a new development task (i.e. on something like Jira), or it is completed before the PR merges. I'm not sure that having comments in the code surfaces that information in a useful manner. The code is for the code, not for what the code could be. The comments on what it could be should be handled outside the code at a different abstraction layer.
- Aurornis 6mo ago
  Agreed. Putting comments in code is good for adding context around the code, but the actual action item needs to be tracked in the same place that all other action items and issues are tracked. An exception would be for information that doesn't yet qualify as an action item, but could become an action item if someone changes something in the code. Like if removing a conditional check would trigger the need for some other work or a refactor. Then it's good to put it close to the code so anyone touching that code will know they need to make the action items if they go down that route.
- waynesonfire 6mo ago
  > new development task (i.e. on something like Jira), or it is completed before the PR merges. You've never written an TODO comment? I find them useful.
  - christofosho 6mo ago
    Perhaps in a small codebase without issue tracking this might be something leverage. It's just not reasonable with 100k+ lines of enterprise grade code.
flossly 6mo ago
When I review a PR, I don't add comments to the code. If I think something needs to be commented: I comment on the PR and reject the PR. If I think --as is discussed in the article-- that the comment "would be nice, but is not strictly necessary", then I comment on this on the PR and approve the PR.

Someone1234 6mo 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 6mo 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 6mo ago
  You get to design committees where everything has to be approved in advance.
  - Someone1234 6mo ago
    Yep, where productivity goes to die and your developers feel no autonomy/trust.
ljm 6mo ago
Usually by the time a PR has been submitted it's too late to dig into aspects of the change that come from a poor understanding of the task at hand without throwing out the PR and creating rework. So it's helpful to shift left on that and discuss how you intend to approach the solution. Especially for people who are new to the codebase or unfamiliar with the language and, thanks to AI, show little interest in learning. Obviously not for every situation, but time can be saved by talking something through before YOLOing a bad PR.
- nightpool 6mo ago
  Yes, it should be cheap to throw out any individual PR and rewrite it from scratch. Your first draft of a problem is almost never the one you want to submit anyway. The actual writing of the code should never be the most complicated step in any individual PR. It should always be the time spent thinking about the problem and the solution space. Sometimes you can do a lot of that work before the ticket, if you're very familiar with the codebase and the problem space, but for most novel problems, you're going to need to have your hands on the problem itself to get your most productive understanding of them. I'm not saying it's not important to discuss how you intend to approach the solution ahead of time, but I am saying a lot about any non-trivial problem you're solving can only be discovered by attempting to solve it. Put another way: the best code I write is always my second draft at any given ticket. More micromanaging of your team's tickets and plans is not going to save you from team members who "show little interest in learning". The fact that your team is "YOLOing a bad PR" is the fundamental culture issue, and that's not one you can solve by adding more process.
  - n_e 6mo ago
    I'm not sure what approach you're suggesting? Asking a more junior developer or someone who "show little interest in learning" to discuss their approach with you before they've spent too much time on the problem, especially if you expect them to take the wrong approach seems like the right way to do things. Throwing out a PR of someone who doesn't expect it would be quite unpleasant, especially coming from someone more senior.
embedding-shape 6mo ago
> PRs are the first time your reviewers have seen that change, so there is no opportunity downstream to do the things you're suggesting. Yes, but I'm arguing for that it shouldn't be the first time they hear about that this change is planned and happening, and their input should have taken into account before the PR is even opened, either upfront/before or early in development. This eliminates so many of the typical PR reviews/comments. Figure out where you are going before you start going there, instead of trying to course correct after people already walked places.

swiftcoder 6mo ago

> 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 This is accurate, but it's still an important check in the communication loop. It's not all that uncommon for two engineers to discuss a problem, and leave the discussion with completely different mental models of the solution.

bulbar 6mo ago

> 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. I agree in general and tried to push for a more iterate approach in our team. However, my fear is that this would multiply the effort because it's very easy to discuss many things that do not matter or turn out later to not matter. It seems it's easier for people to only discuss the important stuff when presented as an actual implementation. We are talking tendencies here, of course, general design decision must always be done beforehand.

embedding-shape 6mo ago
> It seems it's easier for people to only discuss the important stuff when presented as an actual implementation. LLMs help a lot here, create two prototypes with both designs, compare them together :) Could even evaluate how ergonomic some future potential change is, to see how it holds up in practice. I used to use pen and paper to essentially do the same, minus having real code and instead just talking concepts, but it does suffer from the issue that some need to be confronted with code in front of them to visualize things better for themselves.

Bridged7756 6mo ago

I think it's a fine line to walk. At my job what we do is discuss any complex implementation or risky change or blockers in the dev eng meeting. For smaller stuff, or more straightforward solutions, we don't bring it up. If you make it a hard rule to first discuss all tickets, it just seems draconian. Code review is specifically for code quality, more lower level stuff.

christofosho 6mo ago

You might be approaching PR comments differently than I've seen. When a comment is something to be addressed, it's either put into a new development task (i.e. on something like Jira), or it is completed before the PR merges. I'm not sure that having comments in the code surfaces that information in a useful manner. The code is for the code, not for what the code could be. The comments on what it could be should be handled outside the code at a different abstraction layer.

Aurornis 6mo ago
Agreed. Putting comments in code is good for adding context around the code, but the actual action item needs to be tracked in the same place that all other action items and issues are tracked. An exception would be for information that doesn't yet qualify as an action item, but could become an action item if someone changes something in the code. Like if removing a conditional check would trigger the need for some other work or a refactor. Then it's good to put it close to the code so anyone touching that code will know they need to make the action items if they go down that route.
waynesonfire 6mo ago
> new development task (i.e. on something like Jira), or it is completed before the PR merges. You've never written an TODO comment? I find them useful.
- christofosho 6mo ago
  Perhaps in a small codebase without issue tracking this might be something leverage. It's just not reasonable with 100k+ lines of enterprise grade code.

flossly 6mo ago

When I review a PR, I don't add comments to the code. If I think something needs to be commented: I comment on the PR and reject the PR. If I think --as is discussed in the article-- that the comment "would be nice, but is not strictly necessary", then I comment on this on the PR and approve the PR.