If you pair, there’s two sets of eyes, to commit both pairs have to sign a commit. You can also organise a demo/quick mob session before commit. Then there’s a level of trust in your teammates.
PR’s are great for open source projects as act as gatekeeper so not everyone can commit freely. If you need to gate keep your team members then I’d question the strength of your team.
The best teams I worked on who delivered working code fast, efficiently even when they are some of the most complex projects I worked on committed straight to trunk, had a very good build pipeline (super import) and worked closely together for review. The standards where extremely high yet the general feeling was it was less dogmatic, micromanaged or kept behind a gatekeeper.
The projects I’ve worked on with dogmatic pr’s generally failed to deliver anything in any amount of reasonable time. The prs where dogmatic as more junior teams get caught up in superficial things such as names, package structure, syntax preferences rather than what the pr actually does.
As one example, do you think cross-functional changes to the Linux kernel from even trusted contributors can just be merged without review and feedback from people across all affected subparts of the kernel, as long as they have been written through pair programming? That's a big open source example, but plenty of companies have plenty of projects for which that already applies as well.
> The prs where dogmatic as more junior teams get caught up in superficial things such as names, package structure, syntax preferences rather than what the pr actually does.
That is an easy trap to fall into (and wildly annoying), but it does not mean that PRs have no use outside of that.
Review also shouldn't be causing ego antagonisms. You're coworkers. You have to be able to work together. That's called having a job.
Every project I have worked on in a professional setting has had mandatory code review, with review acting as a gate. That's been required in order to maintain software quality despite the quality of engineers I've worked with.
All this means is that you and your team failed to learn anything from the PR process. If simple things like naming or syntax repeatedly come up then you have a style convention but the devs are ignoring it. The very obvious solution to that is to enforce the rules with a linter, and run the linter on a commit or push hook. If the linter fails then the dev can't open the PR until they fix the issues.
If you have process that's only for box checking and not something that's actually providing useful data the you're not using your time well enough. Removing the process is one solution, but it's not a very good one. Making the process useful is significantly better.
(Automating code quality stuff with linters is a no-brainer though.)
These two are not superficial, though. Naming things is one of the Hardest Things, and that and structure tell you if you're in the right spot trying to track down a bug or add a feature.
Every debugging session should not be an adventure.
As you write there is much more to it and one can only see through it after joining company.
For me trunk based development alone would be indicator that company is immature and does not even know they can have a process.
Thinking it always does or doesn't make sense just tells me you don't have experience working on diverse projects.
Like you totally disagree.
I described my experience and what I saw, well I did not do any scientific research on 1000s of companies. But I still think my experience has the same validity as your statement. So it can be both at the same time, there is so many companies small and big.
I'm sorry, what?
Review is a gate for everyone, and is a sign of basic project maturity. WebKit, Mozilla, Chrome, LLVM, Linux, etc are all review gated projects. No change is landed - can be landed - without review. If you're questioning the strength of those teams I cannot imagine what your team would need to have on it??
And pair programming? No thanks.
I think it you are doing actual code reviews, you are doing something wrong.
Smaller companies can pay much more (in expectation), and can simply poach the cream of the crop (who are invariably frustrated by useless process and corporate politics) from the big ones.
On top of that, most development processes are designed to minimize the blast radius of underperforming engineers, so your actual bar is "hire stronger engineers than the dregs from the giant shops".
Because in my experience, reviews and PRs certainly Ven damage latency, but overall throughput remains the same as it would be with subsequent follow-up fixes of these issues to the trunk.
Everyone is “gated” on a code review. The PR is one mechanism by which you can make code reviews easy. It says nothing about the strength of the team.
Yeah exactly, PR's are based on the fact that you have some person who is the owner that have complete power, and many other contributors who have zero power and whose contributions will mostly be rejected. You simply don't have that situation in a company, where everyone is an owner on equal terms, and all contributions are accepted, only some with slight modifications.
So you get these really weird situations where more junior, or less skilled, people can block PR's and demand changes from other more skilled and/or senior people. And if there's disagreement in a review: the resolution of that is completely unknown, and can often blow up to a really nasty conflicts.
I have so many terrible experiences with PR's where you get hostile nonconstructive comments on your work, on github, from a person that literally sits next to you. It's the creepiest thing ever.
PRs can be approved based on two people's opinion. There doesn't need to be a central gatekeeper.
> So you get these really weird situations where more junior, or less skilled, people can block PR's and demand changes from other more skilled and/or senior people.
Sometimes junior, or less skilled people, have something valuable to say. Especially if the code could be simpler.
In a stalemate, the PR could be sent to a third party. I've suggested this many times to avoid unnecessary conflict.
I don't think it is PRs that are the issue, rather your working environment.
Yeah and sometimes they are naive, dogmatic and overconfident, and on a crusade to change all the things! because they have read some blog post by uncle bob, and this tool is putting them in absolute power every time they do a review.
> In a stalemate, the PR could be sent to a third party. I've suggested this many times to avoid unnecessary conflict.
Ok and who might this lucky scapegoat be? I have a feeling it's not the manager for some reason..
> I don't think it is PRs that are the issue, rather your working environment.
The issue, which I'm trying to illustrate, is that the tool makes the working environment worse by introducing (hostile) dynamics between people that don't exist, which leads you into situations that you don't have resolutions for, situations that should not occur.
Using a tool that allows you to block other people's work causes unnecessary conflict in a team where people are supposed to be working together.
Edit: blocking contributions is a normal and natural thing in an open source workflow, and it is not normal and natural in a team inside a company.
IMO every developer should be an admin of the repo, but more importantly it’s not a gate, it’s a process that benefits communication. All the team can read and learn from small addings to the code base.
PR reviews should not equate to 100 comments either, complex discussions can (should imo) be worked out in a call discussing (dialog) best approach. Think of white boarding a problem with a fellow engineer friend.
Ofc, teams differ and some teams work better with other processes and other tools.
It's the tool itself and it's imposed workflow of blocking work and gaining absolute power in demanding changes, that causes the working environment to become toxic. Nobody would ever do that in a meeting "I'm blocking this work now until my demands have been met". That would be incredibly hostile, but with this tool it becomes normal.
