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

41–50 of 77 posts

Re: Commenting and approving pull requests

#41

Post by fishtoaster »

punkbit wrote: post #28

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 , forcing people to stick with Array for simple types like number[] even though TypeScript's own docs and tooling push…

Yeah, I think all style comments should be handled by either a linter/formatter or be written in a style guide. Everything else is up to personal preference.

Then, when a style comment comes up in a PR, the answer is "Oh, do you think we should add that to our style guide? If so, let's discuss that in slack. Until then, though, that's not blocking."

Re: Commenting and approving pull requests

#43

Post by TheGRS »

Quarrelsome wrote: post #33

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

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.

Re: Commenting and approving pull requests

#44

Post by TheGRS »

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.

Re: Commenting and approving pull requests

#45

Post by philipnee »

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.

Re: Commenting and approving pull requests

#46

Post by n_e »

nightpool wrote: post #19
ljm wrote: post #18

Earlier quoted context omitted.

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…

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.

Re: Commenting and approving pull requests

#47

Post by musicmatze »

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

Re: Commenting and approving pull requests

#48

Post by lexicality »

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.

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.

Re: Commenting and approving pull requests

#49

Post by dbalatero »

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

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

Re: Commenting and approving pull requests

#50

Post by motoroco »

andrewjf wrote: post #34
4lx87 wrote: post #24

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

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…

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