Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Commenting and approving pull requests

jakeworth.com

Post reply on HN
Print view View original

11–20 of 77 posts

Re: Commenting and approving pull requests

#11

Post by swiftcoder »

embedding-shape wrote: post #6

> 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 th…

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

Re: Commenting and approving pull requests

#12

Post by bulbar »

embedding-shape wrote: post #6

> 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 th…

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

Re: Commenting and approving pull requests

#13

Post by jez »

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

Re: Commenting and approving pull requests

#14

Post by bulbar »

Someone1234 wrote: post #10
embedding-shape wrote: post #6

> 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 th…

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.

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

Re: Commenting and approving pull requests

#15

Post by yojo »

adamgordonbell wrote: post #8

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.

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

Re: Commenting and approving pull requests

#16

Post by yojo »

jez wrote: post #13

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

You might try adding this branch protection rule to require conversation resolution: https://docs.github.com/en/repositories/configuring-branches...

Re: Commenting and approving pull requests

#17

Post by esafak »

bulbar wrote: post #14
Someone1234 wrote: post #10

Earlier quoted context omitted.

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.

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

You get to design committees where everything has to be approved in advance.

Re: Commenting and approving pull requests

#18

Post by ljm »

Someone1234 wrote: post #10
embedding-shape wrote: post #6

> 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 th…

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.

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.

Re: Commenting and approving pull requests

#19

Post by nightpool »

ljm wrote: post #18
Someone1234 wrote: post #10

Earlier quoted context omitted.

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.

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 learnin…

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.

Re: Commenting and approving pull requests

#20

Post by embedding-shape »

bulbar wrote: post #12
embedding-shape wrote: post #6

> 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 th…

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

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

Post reply on HN