171–180 of 233 posts
Earlier quoted context omitted.
If I read it right, it described there being a "code review team." Whether they're responsible for reviewing all code, or just for coming up with these sorts of practices was not clear. ETA: with that said, the "reviewer recommender" to me would imply it's people who work on the same codebase.
Nah, a dedicated "code review team" is not a thing. Often the suggestion featured does suggest a whole team as a review based on code ownership.
As the commit author, it's in my (and everyone's) interest to size changes up so that it's easier for review, and also present in them in the logical order of thinking. Personally, I prefer the bottom-up approach.
I bring the non-functional and impertinent changes (like refactoring and tangential changes) ahead in the line-up so that the actual changes are kept separate and are concentrated at the tail end.
I make commit messages of the pattern: Present situation, the problem with that, what this patch does, and what the effect it has/how it solves the problem or sets up a path forward.
The initial PR might be sliced too thinly, and so will have more commits than ideal. But, as the review progresses, and once both the reviewer(s)' and the author's mental models are in sync, commits can be collapsed at their logical boundaries.
Regardless of the tooling and presentation, it's imperative that that the reviewers are intuitively aware of the ramifications of the change. Without that, the review ends up being nit picking, spell checking, and whatever that's obvious on the immediate vicinity, and the process degrades into a box-ticking exercise.
No AI needed. Be human.
Earlier quoted context omitted.
I work on similar sized teams, I introduce my PRs as stacked so they are extremely consumeable in very small bite-sized code changes and build on the last one reviewed.
Do you use any special tools for this? I've heard of people "stacking" diffs in this way, but it seems like it would be clunky to review on GitHub.
Have I missed the feedback from the users? There should be some quotes from team members who liked the change. Their mentioning that they start being data-driven for internal tools suggests that they start treating developers like cattle and not pets. >Driving down Time In Review would not only make people more satisfied with their code review process, it would also increase the productivity of every engineer at Meta…
I think there's assumption that people won't just rubberstamp significant diffs to code they don't own. When submitting a change to another team's project if the reviewers that are suggested aren't actually the right person they are more likely to know the right person who should review it and they can manually add that person.
>The 1.5 percent in reviewed diffs could also be jitter.
Facebook / Meta has tools for measuring the effects of changes and seeing if they are statistically significant. Yes, it could still be jitter but without them giving more data about the experiment we can't tell what the chance of it being due to chance is.
>There doesn't seem to be a review process for the review team
There isn't a review team. Anyone can review a change.
Am I reading this right? If the total median time in review is “a few hours”, that means that people are dropping what they’re doing to review the code. That can’t really be a good code review? A context switch alone would be half of that time for me.
I work at a company with a similar code review culture. I have about 2 blocks of meetings per day on average. I usually check my assigned code reviews in the morning and when I come back from the meetings. Once I’m done with those then I move on to my own work. If I have too much code review to the point that I can’t get my own work done, I take my name off the reviewer list and it’s reassigned. On most days I’m stil…
> Next reviewable diff Holy Fuck No. 90% of my PR review time goes into "Okay, how will this change impact parts of the system that this mid-level Dev doesn't understand yet?" and "Does this PR actually do what the ticket it is claiming to implement actually intended?" Reviewing diffs in isolation completely removes one's ability to do that. If you remove a person's ability to do that, what you've left them with is t…
Earlier quoted context omitted.
Working there I tended to conduct code reviews each afternoon at the end of work. Sometimes after coming back from lunch as well. There isn't any "drop what they're doing" involved.
Sure, I do reviews first thing every morning, and sometimes right after lunch (mostly just rereviews), but that would give a mean/median response time of 4 hours assuming work completion time is uniformly distributed. And if changes are requested, that would add another 2-4 hours, which brings the total review-in-wait time to a full day, which the post was saying was unacceptable. To get the numbers they claim they m…
Am I reading this right? If the total median time in review is “a few hours”, that means that people are dropping what they’re doing to review the code. That can’t really be a good code review? A context switch alone would be half of that time for me.
I work at a company with a similar code review culture. I have about 2 blocks of meetings per day on average. I usually check my assigned code reviews in the morning and when I come back from the meetings. Once I’m done with those then I move on to my own work. If I have too much code review to the point that I can’t get my own work done, I take my name off the reviewer list and it’s reassigned. On most days I’m stil…
Are you sure you are a developer?
Earlier quoted context omitted.
Sure, I do reviews first thing every morning, and sometimes right after lunch (mostly just rereviews), but that would give a mean/median response time of 4 hours assuming work completion time is uniformly distributed. And if changes are requested, that would add another 2-4 hours, which brings the total review-in-wait time to a full day, which the post was saying was unacceptable. To get the numbers they claim they m…
You can use timezones to your advantage here. When California is finishing, Singapore is starting and when Singapore is finishing London is starting.
Earlier quoted context omitted.
Boo, please consider a review of the HN comment guidelines https://news.ycombinator.com/newsguidelines.html Could you potentially explain what makes it "some of the most garbage software imaginable"? edit: wording
There's already many articles posted to hackernews frequently at the insane internals of the mobile app, to the "meta verse", to many other things. Most of which are posted by ex-Facebook engineers. I don't need to rehash the same conversations, nor do I need to grant Facebook of all corporations any sort of courtesy.