alt Hacker News> 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 them right in the code is much more evident to all the people passing through, or even reminding yourself in the future too.
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. If there is so many unknowns first, do one step to figure out how it can be done, then agree on that, again before starting the actual work.
That leads to PRs basically just being spot-checking, not deep analysis nor even space for nitpicks. 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.
Replies
> 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.
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.
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.
> 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.
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.