201–210 of 233 posts
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.
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.
Pairing can sidestep both of these problems. No context interruption and reviewed continuously.
Earlier quoted context omitted.
A diff at meta is a pull-request.
No. A diff is a commit.
I don't work at Meta and I still don't have any idea which of you is right.
A commit would satisfy the description above.
As would a pull/merge request, which the image in the article perhaps fits better: https://engineering.fb.com/wp-content/uploads/2022/11/Code-R...
Of course, it's possible to review each individual commit as well, but who does that?
I know that some folks love to mess around as much as they need in their local branches, commit often and then do an interactive rebase, to maybe end up with one or just a few commits that can then be the basis for a pull/merge request, then the difference matter a little bit less.
I made a talk about it, unfortunately in Hungarian, but you can see screenshots how it worked: https://youtu.be/7WiICWyP1sQ
Here is the code:
Earlier quoted context omitted.
Quoted post unavailable.
Name one successful company that only produces beautiful, elegant, and readable code, with no nasty warts or hacks. I'll wait.
Honestly, I'd genuinely like to hear some of the answers to this, or even just mentions of good codebases to explore.
That said, I don't think that there will be that many that satisfy all of the requirements listed here, because as the scope of a project grows, there will almost inevitably be some technical debt or accidental complexity, or even subjective things.
Earlier quoted context omitted.
Variation on 2 - We don't care about commit messages at all . All PRs must have good descriptions which use a company-wide template. But individual commits get squashed and their commit messages entirely replaced by the PR template contents. So my commit history is "fump" "GAH" "I think this works?" etc. but all changes have a description of the issue (and link back to the ticket) the solution described in some detai…
Is there any tooling you use to squash the commits and use the PR message?
Just checked, it doesn't seem that my self-hosted Gitea instance allows for that, so neither does Gogs (or maybe I just can't find the option in the UI).
Earlier quoted context omitted.
None of them engage with the content of the article either - having a "play next" button for code review is awesome assuming it works reasonably well. I'm curious about the quality of nudgebot reviews. In my experience the PRs that sit around forever are the 3000 line epic find/replace refactors all done in one commit that are impossible to really review. I skimmed the paper and didn't see any accounting for "diff ti…
what stops you from reviewing the sed one-liner that created the diff, instead?
When I was there, you could always just put Reviewed-By: self in the commit, and not wait. Much faster ;)
> 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…
The fact that this comment still remains at the top here despite being incredibly inaccurate (and many subcomments stating as such) shows how degraded the discussion has gotten here when Meta is mentioned.
Earlier quoted context omitted.
It encourages looking quickly at small bits of changes in isolation as opposed taking a holistic view of the entire change, and viewing them in series as 5 min little tasks you can tick off like tiktok videos. The review tools on github already go too far toward this by removing far too much context.
Meta deploys small changes continuously. If you have a bigger change, it should get broken up into smaller easily reviewable atomic changes.
Imagine a feature that would heavily degrade performances as a whole, but as it’s being introduced in small bits and pieces no one sees the big picture and are left to wonder why perfs are slowly degrading, boiling frog style.
