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

61–70 of 77 posts

Re: Commenting and approving pull requests

#61

Post by Aurornis »

christofosho wrote: post #38
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…

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

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.

Re: Commenting and approving pull requests

#62

Post by allendoerfer »

namenotrequired wrote: post #57

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.

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

Re: Commenting and approving pull requests

#63

Post by waynesonfire »

christofosho wrote: post #38
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…

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

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

Re: Commenting and approving pull requests

#64

Post by namenotrequired »

yojo wrote: post #15
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

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

Re: Commenting and approving pull requests

#65

Post by flossly »

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…

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.

Re: Commenting and approving pull requests

#67

Post by tecoholic »

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.

Re: Commenting and approving pull requests

#68

Post by [deleted] »

philipnee wrote: post #45

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.

[deleted]

Re: Commenting and approving pull requests

#69

Post by [deleted] »

philipnee wrote: post #45

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.

[deleted]

Re: Commenting and approving pull requests

#70

Post by Terr_ »

philipnee wrote: post #45

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.

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.
Post reply on HN