I approve of it.
I have now commented on it.
Commenting and approving pull requests
21–30 of 77 posts
Re: Commenting and approving pull requests
#22> 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.
> 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.
Re: Commenting and approving pull requests
#23This 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 many times I got no response/acknowledgement on a comment that actually surfaced something critical. I haven't been keeping track, but I think my comments could have prevented dozens of outages at this point. It's quite exhausting. In my own experience, the worst offenders of this are senior devs.
> Why approve, if I’ve left comments that I think are worth implementing? > > Because I trust my team. I know that my comments will be considered, and if they’re useful, implemented.
I do this a lot too. It's critical that PR authors don't burn that trust either. If they make substantial changes that warrant another review, I hope they do request it. Too many times in my career have colleagues just went ahead and made bad changes after my approval that I would have easily caught, merge, and things go
High trust, high alignment environments move so fast, and you know when you're in one and know when you have your colleagues' trust. It feels really good!
Re: Commenting and approving pull requests
#24I 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 that require every change have an approval, but don’t require 100% test coverage. I’m seeing senior engineers have to get approval from juniors for a copy change.
It’s theatre. It’s bad management. It’s a cargo cult. It isn’t actually driving code quality.
Code review is one thing. Code review is good. But requiring every change have an approval is something else.
Re: Commenting and approving pull requests
#25Totally agree, and I go even farther: I consider that the PR author is totally responsible of merging their PR or not, and so no comment is blocking. I can make a comment I consider blocking, but there might be very good reasons that I can't see to still merge the PR. Fixes can come in a follow-up PR. As long as everyone takes responsibility for their work, it is a well-working system.
Re: Commenting and approving pull requests
#26> 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 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.
Re: Commenting and approving pull requests
#27This 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?
Re: Commenting and approving pull requests
#28I 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 T[].
Re: Commenting and approving pull requests
#29I 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…
Same. I've said my peace, but I don't let that get in the way of what works.
Notably, prevent github merges if there are unresolved comments, so you know they glanced over them before collapsing it.
Re: Commenting and approving pull requests
#30I 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.