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

71–77 of 77 posts

Re: Commenting and approving pull requests

#71

Post by christofosho »

waynesonfire wrote: post #63
christofosho wrote: post #38

Earlier quoted context omitted.

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.

Perhaps in a small codebase without issue tracking this might be something leverage. It's just not reasonable with 100k+ lines of enterprise grade code.

Re: Commenting and approving pull requests

#73

Post by phyzix5761 »

jjmarr wrote: post #52
phyzix5761 wrote: post #51

With a robust enough test suite and a team that does TDD and mob programming, code reviews are pretty much obsolete and a waste of time. Everyone's already involved in the coding process as a mob and the tests catch any regressions.

There's two purposes to review. One is to enforce ownership, and the other is to provide mentorship. Even if you don't have ownership, mentoring is still useful. I get asked for review by teammembers on an area I have expertise in. Their solution might work but cause problems later. With review, I can knowledge transfer my lived experience so they don't suffer like I did. The third purpose to review is stylistic nitp…

Yes, I agree, but when the whole team is already part of the development process through mob programming there's no one who hasn't seen the code, hasn't provided feedback/mentorship, and hasn't voiced their stylistic concerns by the time the development is done.

Re: Commenting and approving pull requests

#74

Post by jjmarr »

phyzix5761 wrote: post #73
jjmarr wrote: post #52

Earlier quoted context omitted.

There's two purposes to review. One is to enforce ownership, and the other is to provide mentorship. Even if you don't have ownership, mentoring is still useful. I get asked for review by teammembers on an area I have expertise in. Their solution might work but cause problems later. With review, I can knowledge transfer my lived experience so they don't suffer like I did. The third purpose to review is stylistic nitp…

Yes, I agree, but when the whole team is already part of the development process through mob programming there's no one who hasn't seen the code, hasn't provided feedback/mentorship, and hasn't voiced their stylistic concerns by the time the development is done.

If your team is 5 people then sure, it's fine.

From my experience in a massive shared repo with unclear ownership...

Re: Commenting and approving pull requests

#75

Post by ljm »

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 don't disagree that a practical spike is a good way to grasp a novel problem (or work with a lack of internal knowledge because it's legacy code) but there is still something to be said for attempting to work things out in the abstract too, and not necessarily by adding process, but by redeveloping that internal knowledge and getting familiar with the business domain.

In a greenfield project I will have a lot of patience for a team that doesn't grasp the problem space too well yet, and needs to feel around it by experimenting and prototyping. You have to encourage that or you might not even be building anything innovative.

For the longer term legacy project then the team can't really afford to have people going down rabbit holes and it's more beneficial to approach things in the abstract and reduce the problem as much as possible. Especially with junior or mid-level engineers who can see an old codebase as a goldmine for refactoring if left unattended.

As for the fundamental culture issue... maybe. AI increases the frequency of low quality PRs and puts a bigger burden on the reviewer. I can live with this in the short term if people take lessons from it and keep building up their own skillset. I feel this issue is not unique to my team and LLM-driven development is still novel enough that we're all figuring out the best way to tackle it.

Re: Commenting and approving pull requests

#76

Post by ljm »

n_e wrote: post #46
nightpool wrote: post #19

Earlier quoted context omitted.

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

This is how I try to approach it. I don't think it's a new thing for a new hire to come in hot and try to figure things out themselves rather than spending time with the team. Or getting lost down rabbit holes.

Re: Commenting and approving pull requests

#77

Post by namenotrequired »

allendoerfer wrote: post #62
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.

New code has bugs regardless of whether it entered on the same PR or a new one
Post reply on HN