| ▲ | atomicnumber3 3 hours ago | ||||||||||||||||||||||||||||||||||||||||
If only one person knows the code, then PR time isn't going to save you. I'm tech lead and I basically don't review PRs, and I tell people this, with a caveat - if you can tell me what you specifically want me to review, for what specific purpose, I'm happy to! So "can you review this bit for race conditions" is great, love it. This forces people to actually think about what in their code they should be suspicious of, if anything. "Can you review this" [link to PR] is getting a rubber stamp because humans have never been good enough at "just spotting bugs" to make this worth it and now any LLM is better than a human. And for the purpose of understanding - PR time is too late. I have not reviewed a PR ("for real") in a long time and yet I could tell you how every system my people have built works down to a very fine level of detail. And it's because _we talk to each other!_ We don't just chill in the same slack channel and code independently, we all value each others brains and want each others inputs because we know it will improve our product and we value what perspectives others will bring. Trying to learn via PR is a sad substitute for real collaboration and teamwork. | |||||||||||||||||||||||||||||||||||||||||
| ▲ | 20k 18 minutes ago | parent | next [-] | ||||||||||||||||||||||||||||||||||||||||
This is wild to read, I always review PRs and frequently find bugs or significant problems in them that get them bounced back | |||||||||||||||||||||||||||||||||||||||||
| ▲ | setr 3 hours ago | parent | prev | next [-] | ||||||||||||||||||||||||||||||||||||||||
I consider PRs to be primarily a defense of the architecture, and to a lesser degree a general sanity check. It’s also a useful opportunity to enforce automations are being run | |||||||||||||||||||||||||||||||||||||||||
| |||||||||||||||||||||||||||||||||||||||||
| ▲ | infinitebit 2 hours ago | parent | prev | next [-] | ||||||||||||||||||||||||||||||||||||||||
THANK YOU. I’ve always felt this way about PR review. I feel like people should write a natural language description of the change, and every section of it should link to part of the diff, and every part of the diff should be linked to by part of the description. That or just leave a comment on every chunk of the diff. | |||||||||||||||||||||||||||||||||||||||||
| |||||||||||||||||||||||||||||||||||||||||
| ▲ | skydhash 2 hours ago | parent | prev | next [-] | ||||||||||||||||||||||||||||||||||||||||
I follow mailing lists (emacs and openbsd) and sending a diff is kinda the boundary between wishing for something and making the something into a thing. It’s the difference between discussing a plot and discussinf a draft. Sneding a PR should not be for understanding or just for rubber stamping. It’s about getting someone to look at your approach and helping you find flaws or proposing ideas that could make it better. When I review PR, the primary question is: For the stated problem, is the diff a good solution? Sometimes I don’t know enough about the problem, so I just try to see if the code has glaring mistakes (mispellings, styles,…) but those are just comments, not suggestions. | |||||||||||||||||||||||||||||||||||||||||
| ▲ | vcryan 3 hours ago | parent | prev [-] | ||||||||||||||||||||||||||||||||||||||||
Well articulated. Ty | |||||||||||||||||||||||||||||||||||||||||