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

31–40 of 77 posts

Re: Commenting and approving pull requests

#31

Post by __alexs »

epgui wrote: post #30
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…

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.

Write a lint rule. It's often quite easy.

Re: Commenting and approving pull requests

#32

Post by Someone1234 »

esafak wrote: post #17
bulbar wrote: post #14

Earlier quoted context omitted.

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

Yep, where productivity goes to die and your developers feel no autonomy/trust.

Re: Commenting and approving pull requests

#33

Post by Quarrelsome »

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

Re: Commenting and approving pull requests

#34

Post by andrewjf »

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

Re: Commenting and approving pull requests

#35

Post by gedge »

jt2190 wrote: post #27
gedge wrote: post #23

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…

Isn’t a change to the code that incorporates your suggestions enough acknowledgment? Presumably the code change would be required to get your approval?

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.

Re: Commenting and approving pull requests

#36

Post by punkbit »

epgui wrote: post #30
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…

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.

It's quite simple to write a rule, e.g. for the example I shared 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.

Re: Commenting and approving pull requests

#37

Post by Pannoniae »

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…

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?

Re: Commenting and approving pull requests

#38

Post by christofosho »

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 handled outside the code at a different abstraction layer.

Re: Commenting and approving pull requests

#39

Post by theowaway213456 »

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

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.

Re: Commenting and approving pull requests

#40

Post by james_marks »

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…

This is exactly the pedantry I try to avoid in my teams.

The point of a review shouldn’t be to make sure it’s exactly how the reviewer would do it. Is it safe? Does the author understand the area? Is there evidence the code has been exercised? Does it follow the major conventions?

Beyond that, I try very hard to build a culture of approvals vs nitpicking, and let the linters enforce the rest.

Post reply on HN