by bigblind on 4/24/23, 7:37 AM with 98 comments
by coldtea on 4/24/23, 9:10 AM
This guy (not the post author, but Dave Farley, the guy who wrote the above excerpt) has never heard of "trust but verify".
PR is about verifying, having another set of eyes for anythine the original dev might have missed, giving an opinion before something on a branch goes into permanent git history, and so on.
This is completely orthogonal with whether you trust your team members or not. Besides "blind trust" is not the only form of trust.
by alex-moon on 4/25/23, 7:43 AM
The way I'd put this is: writing code and reviewing code are - not two different skillsets maybe, but - two different headspaces. Code review is a kind of "technical QA", and indeed I am pretty sure that you see the same basic practice in factory work (where we get the concept of "QA" from).
by the_gipsy on 4/25/23, 6:59 AM
I've seen teams that try to mimic OSS development and only merge absolutely perfect PRs, dragging out long-living branches and integration. At some point the perfectness can't keep up with the plan, huge PRs are suddenly reviewed hastily, and the end result is worse and slower.
Bottomline: PRs can be used slightly differently in companies to improve speed and reduce overhead.
by eikenberry on 4/25/23, 6:50 AM
by kerkeslager on 4/25/23, 8:03 AM
> "I think this is really bad way to organise a development team.
> "If you can't trust your team mates to make changes carefully, then your version control system is not going to fix that for you."
Man, I don't trust myself to be perfect. I trust myself to be careful but no amount of care makes me infallible. I make mistakes. And if we can catch my mistakes early by getting a second set of eyes on them, they're less painful for everyone to fix. When someone spends their valuable time catching my mistakes, they're doing me a favor.
Distrust might be part of the origin of pull requests, but that doesn't have to be the reason why your team uses pull requests. It can be about catching each others mistakes to learn, create a better product, and get better rewards. The tool is yours to use how you see fit--don't let ego prevent you from making it useful.
by Cthulhu_ on 4/25/23, 7:44 AM
I don't trust myself either; trusting your own or others code is hubris. The two-person rule - or more - is applied EVERYWHERE.
by pyrale on 4/25/23, 8:16 AM
> If you take away the appeal to trust, though, there isn't much left of the argument. What remains is essentially: Pull requests were invented to solve a particular problem in open-source development. Internal software development is not open source. Pull requests are bad for internal software development.
The specific point he tries to reword exists to explain that not everyone needs as much trust than open-source, and, therefore, that not having PR isn't a fatal flaw outside of open-source. The people who use it do so within a discourse along with other points explaining what they gain from forgoing PRs (usually, better continuous integration).
I don't buy the whole discourse about not using PRs, but I'm also not sold on the idea of dismantling a coherent discourse into separate talking points and demanding that they should stand separately. I'm also not a fan of rewording someone else's point and slipping a big non-sequitur in that rewording.
by madsbuch on 4/25/23, 8:15 AM
The reasoning behind this is exactly that we trust individuals on the team to merge. and because reviewing in the traditional sense seems to be a formalisation of distrust: Something you do when you don't trust your peers to implement and put in production properly tested code.
by onion2k on 4/25/23, 8:16 AM
If you can trust the team with one and not the other then something is wrong. There shouldn't be any "this way is better than this other way" argument. Your team should be capable of doing both. If it isn't then neither will work very well. You're going to have problems, conflicts, bugs, etc no matter what you do.
Believing that the team should stop PRs because they're too slow, or they should stop trunk-based dev because pairing takes too much effort, are both red flags. They're both saying that the team has a problem to address.
by lone-commenter on 4/25/23, 7:21 AM
> First, it includes an appeal to trust, which is a line of reasoning with which I don't agree. You can't trust your colleagues, just like you can't trust yourself. A code review serves more purposes than keeping malicious actors out of the code base. It also helps catch mistakes, security issues, or misunderstandings. It can also improve shared understanding of common goals and standards. Yes, this is also possible with other means, such as pair or ensemble programming, but from that, it doesn't follow that code reviews can't do that. They can. I've lived that dream.
I think that the other counter-argument discussed in the article can't confute Farley because, contrary to the author's stated intentions, it is in fact a straw man. The author focuses on the benefits of "asynchronous" workflows over "synchronous" ones: i.e., in his words, reviewing pull requests is easier (for an introvert like himself, at least) than dealing with the social stress involved in pair/ensemble programming.
by usrusr on 4/25/23, 9:10 AM
PR are not only about placing a bouncer at the club door to, figuratively speaking, check entering members for excessive drunkenness, they are about placing a bouncer at the door to give access to a wider group.
This is quite obvious in open source, where the PR model has been a revolution in terms of lowering barriers of entry. You can join the party without asking for the keys. But similar barriers exist in companies. When they don't exits on the technical level (they usually do, sometimes to the point where collaborating teams wouldn't even know what version control the other side is using), they exist on the social level and a model like PR/MR can help nudge both towards the "internal open source" mindset.
by dwheeler on 4/25/23, 1:48 PM
by zajio1am on 4/25/23, 4:21 PM
by BerislavLopac on 4/25/23, 4:36 PM
by muskmusk on 4/25/23, 2:22 PM
If doing a pr takes longer than 5 minutes in general then consider if you are relying too much on pr's.
by teekert on 4/25/23, 8:10 AM
That’s how we treat PRs anyway. Occasionally I or a colleague indeed accepts their own PR (or use direct pushing) if it’s really minor (think a doc string) and need to get something out. So PRs are not just for envs without trust.
by quadhome on 4/25/23, 8:06 AM
If you’re interpreting this through the lens of a single main/master/trunk and multiple branches, then like both Farley and the author, you’re trying to interpret a foreign culture (distributed version control) through a parochial lens (single trunk-multiple branch).
The DVCS world is one of many sources of truth. For example, in the kernel community, Red Hat, Google, Debian, Android, etc. all have their own branches. Linus has a particularly popular branch. None of them are the main branch.
by flohofwoe on 4/25/23, 7:04 AM
And as an UI workflows which 'formalises' the discussion that needs to happen around a merge anyway they are quite nice.
(e.g. without PRs you still need to talk about "hey, I'm going to do this thing" => "hey this thing is ready" => "ok, let's merge this thing")
by jabradoodle on 4/25/23, 8:14 AM
If no one is to review the code, then all code is either written by pairing/mobbing, or else it is the product of one persons thinking, blindspots, biases and all. It's great if you work in a team that pairs in everything, does trunk based development etc; that doesn't make it suitable for every team.
by zabil on 4/25/23, 7:55 AM
All merges to main is deployed across environments so pull requests help us get faster feedback if something fails.
I think pull requests get a bad rep because it is thought as a review process. We see them as short lived branches. They actually improve code quality and unwanted commits even without manual review/approval.
by fmajid on 4/25/23, 8:38 AM
by Illniyar on 4/25/23, 2:18 PM
The way it was done was to have QA check your code, usually as part of a release.
I think PRs became more popular in non-OSS because of CI/CD and agile methodologies.
by someweirdperson on 4/25/23, 7:37 AM
But what else could be a nice short term for "collection of changes with technically enforced reviews and tests"?
by papito on 4/25/23, 1:40 PM
Uh... Pull requests are not a version control workflow - it's a human workflow. It's also a way for the team to actually LOOK at the code that is going in and to be aware of what changes are incoming.
I mean, that's just one point, there are so many ways in which this view is fatally flawed, I am not even going to... ugh.
I know this is not trolling, but this is so scandalous, it comes across that way. This is beyond a "hot take".
by javier_e06 on 4/25/23, 3:15 PM
In the corporate world is more like Seinfield's Soup Nazi skit.
We are all working towards AI accepting or rejecting code changes. Humans have egos and their idiosyncratic bias/baggage get in the way of adopting or rejecting code changes in an objective manner. Githook 50/72 rule anyone?
by manuelabeledo on 4/25/23, 2:26 PM
Am I missing something, or is the irony lost on this one.
by Lapsa on 4/25/23, 7:23 AM
by gorgoiler on 4/25/23, 6:53 AM
This topic reminds of something Joel Spolsky told me about Stack Exchange, in the context of social networks. Giving users the ability quote text when replying is something they really wanted to avoid because it causes arguments to devolve into semantics — far more so than if people had to write out responses in full. A tool like quote-replying shapes (and deforms) our ability to have productive debate.
The unit of change in software is a patch and yet most of us use completely different tools for writing patches, reading our own patches, and reading other peoples (IDE/editor, git diff/show, and GitHub/Lab respectively.)
I like the change of context that web based patch review gives me. I often find bugs or untidiness when looking at my own changes in a browser. When working with others, I’ve found the best code review to be about collaboration and sharing of ideas and responsibility, picking out unseen errors, adding overlooked refinements, giving advice, and improving the team’s bus factor.
If we’re supposed to be playing co-op instead of death-match, shouldn’t we all be in the same level?