Hacker News Reader

Comment by BFLpL0QNek | Hacker News Reader

Skipping pr’s is not equal to skipping code review.

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.

View on HN

I'm sorry, no, there's plenty of code that requires much more than two sets of eyes, and outside discussion, for any project above a certain size. I trust your experience that the teams and projects worked out like that, but they must have been suitable to that approach, which is definitely not universal.

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.

Sure there are a 1% of megaprojects that require additional process, but for the rest PRs are a method to control code quality socially. They introduce delays and foster ego antagonisms, so less methodical ways to control quality are optimal if the requirements are met (buy-in + skill) and complexity isn't too great.

No, every project I have worked on has review gated commits. It is a /basic/ step in ensuring that a project maintains a high quality codebase. Review does cause delays, because reviewing takes time, but we've generally found that the speed "gained" through poor change control is more than made up for through bad code.

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.

Agreed. I'd like to add that PRs are not only a means to review code to improve that code. They're also way for new hires and jr devs to learn new things, how the hive mind thinks, etc. This is, ideally, today's PRs help improve tomorrow's code as well.

You can have perfectly fine, high quality, peer reviewed code without PRs.

Only if some regulation requires sign offs from e.g.other depts. PRs are inevitable. In all other situations they are at best an inneficient workflow and at worst a Kafkaesque circus.

Peer programming, daily checkups, a rock solid CI, and, above all, trust in the professionalism of your team are some ingredients for high quality, high throughput software development.

This is not an opinion. It's a scientifically proven fact. As laid out in the book Accellarate.

Peer programming is just code review again, except now you're only allowed to write when two people are available.

Indeed.

The point was not that "review" is bad, or should be avoided. But that a Pull-Request is a poor way to do that review. Reviewing is crucial, IMO.

> Peer programming, daily checkups, a rock solid CI, and, above all, trust in the professionalism of your team are some ingredients for high quality, high throughput software development.

Absolutely agree, the collaboration should happen sooner in the process, and I would add that the team should probably also have made at least a high level proposed solution together before the work even started.

People seem to have forgotten what code was like back in the bad old days.

Fuck that dogpile of bullshit.

You've never seen a 1200 line diy function to parse XML have you?

I apologise if I left you with that one when I left my first job in 2003!

When I joined in 2006, someone had replaced it with 21 regular expressions.

It's alright, when I started there I turned those 21 regular expressions into a single enormous regular expression that could only be understood at the time of writing, and never understood by mere humans again.

shudders

> Review also shouldn't be causing ego antagonisms.

Yeah but the tool itself is antagonistic, because it imposes a workflow of open source, a workflow that also is antagonistic with clear and absolute power. So using that tool suddenly brings that antagonism and power into a team which is supposed to be 100% collaborative, and it also only does it periodically and with random and different people in power. It's not hard to see how that becomes somewhat complicated.

Anyone who can't handle having their code reviewed before it goes into production is not welcome on my team. You can call that antagonistic or not, but I have always found that the best engineers are the ones who are able to separate criticism of their code from criticism of themselves. Get that type of people on your team, and code review is no longer antagonistic: it's just an egoless feedback process that improves the end product, which is what all members of an engineering team should be working toward.

I'm not criticising the review itself as much as rather the PR workflow and the tools of github, especially the blocking mechanism. It's my opinion that collaboration should happen much sooner rather than being pushed to the end with the review of a PR, and that you should have designated people who have the power to sign something off as production ready.

If a team inside a company want to gather inspiration from the open source world, they should look at how the owner teams of open source projects work internally, not how they work together with outside contributors.

And, it's a very common "solution" in tech to just simply not have any feelings, but people have feeling and that you can't turn off, and that's a very important contributor to work satisfaction.

There is nothing about github hat prevents you from working that way. I don't see what your issue is. But, I will say that I don't want people who can't separate criticism of their code from criticism of themselves on my team. You can certainly have whatever feelings you want, as long as it doesn't get in the way of producing the best possible product, all things considered.

Yeah there's nothing that prevents you from using a tool in exactly the opposite way that it's designed to be used, but it's also pretty unlikely that it's going to happen or that it's going to be successful.

Just make a PR early and discuss the code as you build and make changes, not sure in what way the tool wasn't made to do that. Then you also get the discussion interleaved with changes, resulting in basically perfect documentation of how the code was made and why.

The only thing PR's lack in that regard is that if the reviewer accepts it then it gets added automatically, while ideally you should re review things after the reviewer has accepted it. That way they wont accidentally accept prototype changes.

I'm beginning to think you've never actually used github. Are you trolling?

The tool does not stop you from working in the way you suggest. Maybe it's not what the engineers who wrote it originally envisioned, but it's both simple and easy to work in the way you suggest.

If I want early feedback on something I create a draft PR and ask for comments. I can then let people consider my approach asynchronously, in their own time.

> I don't want people who can't separate criticism of their code from criticism of themselves

Yeah good luck with that, nobody can completely separate criticism of their work from criticism of themselves. You are making your job as the team leader way to easy for yourself, "I only hire robots, that's how I solve all these pesky people issues".

In over a decade of enforced code review experience, I've had one developer who was too immature to take feedback. Some folks take it personally and they shouldn't as long as the feedback is about the code. This requires some work on both reviewer and reviewee.

The guy who couldn't take feedback (person A) had code merged in that wasn't properly tested. Person B said, "hey A. I could use some help. We wrote some tests around $feature that were missing and the tests show the feature doesn't work. We see $unexpected results. Wanna make sure we agree on how this should work."

Person A: no, it works, I tested it.

B: could you help us identify the flaw in the tests then - they seem rock solid.

A: no, my code works.

B: ... ok, can you join us and talk through it?

A: no, it works.

A was removed from the team after management came in and A continued to not acknowledge his code could be wrong.

This was aberrational. We, as an org and as a team, constantly strove to keep the focus on the quality of the code. And, yes, his code was borked.

In this example, of course person A is completely in the wrong, but this is a bigger problem of being so immature that you can't admit any fault.

My suggestion is more along the lines of: use a pairing session for review so that you can bring your empathy as well as your technical expertise, and make it a step in the process just like any other steps (testing, PO approval) etc, and just trust people to do it.

I don't think there's any reason to use a tool from open source, to make code review enforced and with passive aggressive online communication and "blocking". Just seems to make work more painful, and less efficient as well actually.

Oh wow, kudos to you, not just for having to deal with that, and getting A removed, but for being so diplomatic about approaching him on the issue.

And yes, someone that lost is pretty rare, but I'd say lower-grade versions of non-transparency and making their work hard to follow is pretty typical (and frustrating).

> nobody can completely separate criticism of their work from criticism of themselves

Yeah I really hate those pesky automated linters running in standardised environments telling me I’ve screwed something up.

(/sarcasm, hopefully clearly!)

and yet many people on this subthread alone have worked in review-required jobs, and have not had a problem with it. They've also provided reviews for other people's patches, and presumably were also able to do it without personal attacks.

It's the bare minimum of professionalism.

If you are unable to separate feedback on your work, from attacks on your person, you are lacking some fairly fundamental skills needed for professional engineering.

> If you are unable to separate feedback on your work, from attacks on your person, you are lacking some fairly fundamental skills

Or for any job role at all?

>So using that tool suddenly brings that antagonism and power into a team which is supposed to be 100% collaborative, and it also only does it periodically and with random and different people in power.

Is code review not collaborative in your experience? In every team I've been on/run we've split code review comments between suggestions/questions and actual "I will not let this go into prod if my name is attached to it" blockers. Code review has been 99% the first set of comments and only rarely do I see anyone actually block reviews over things like style and what not that have been mentioned here.

I can't even imagine those topics getting into code review as a blocker as if we have actually strongly held opinions on mechanical issues like that, they are integrated into the various linters used so you don't even need human eyes to catch the issue.

What tool are you talking about?

I've had patch review as back and forth comments in email, in bugzilla, in myriad other bug databases.

If you can't send out an email with your patch as an attachment, and get feedback, then we have a problem, and the problem is not the adversarial nature of review.

But why would you use such a remote asynchronous late stage feedback loop, if you are literally sitting in the same room as your collaborators, during the whole development process?

> if you are literally sitting in the same room as your collaborators

Your very premise is wrong. At any sufficiently large company, you are unlikely to be sitting in the same room as every collaborator and stakeholder, or even proxies for them.

As a simple example, the team I currently work on (on one project of several) is 10 people across 8 US cities in all four US mainland timezones, and the stakeholders and collaborators are across Australia, Asia, Europe and the Americas. A good majority of what I do is pair programmed, yet the pull request workflow is essential to letting _others_ know what is happening and why, and to allow them to have asynchronous input into the process.

You might argue it would be better for the team to be in a room somewhere. Maybe so, but the people this project demands live where they live and could not even agree a common location for that room to be if they wanted to. And it still wouldn’t help the other projects…

I was more offended by the assumption that you can simply interrupt whatever work your coworkers are doing, just because you're already in a much less pleasant open plan office.

You're not understanding the purpose of review.

The goal in review is to try to catch any oversights or errors in the code you wrote. The code you wrote may have been the result of of discussions with your coworkers, but the code still gets reviewed, even if by people you discussed the implementation with. That review will occasionally find things you missed, and then you can cycle, and re-review.

By definition, the review can only happen at the end of change development.

Have you ever worked on a new codebase?

I've found PR's become important approximately when feature momentum starts to drop.

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.

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.

Even better, identify actual problems you are actually encountering, and design a minimal process to solve those problems.

(Automating code quality stuff with linters is a no-brainer though.)

> superficial things such as names, package structure

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.

Well certainly I agree that naming is important. It’s up there with cache consistency and off-by-one errors as one of the two most difficult problems in computer science.

I would not use trunk based development as indicator of mature team.

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.

I view knowing when trunk based development does and does not make sense as an indicator of a mature team.

Thinking it always does or doesn't make sense just tells me you don't have experience working on diverse projects.

It's the opposite. Few companies know that you can do without PR. Even fewer know why they are doing PR or where it comes from. But everybody starts with PR, it's the default in their VCS UI.

Why it has to be opposite?

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.

Ok, let's say a lot, or most, companies use processes without knowing why.

Some might just push commits without knowing or caring about reviewing code, beyond fixing what fails in production. But others might just do "git flow" or whatever, doing thorough PRs, without knowing that the changes could be requested after being merged and without realizing the amount of time that is wasted on integrating code and re-testing.

In particular, I think that PRs come from an open source model, where you really have to gatekeep. But in a company, there is no problem in following up: the devs are salaried and you just need to put them to work on whatever is necessary.

I agree on that there is a lot of cargo culting in the industry.

I also don't like PR's/Unit Tests/other practices motivated in a way: "because that is what professionals/google/microsoft does".

> If you need to gate keep your team members then I’d question the strength of your team.

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??

The projects listed bear very little resemblance to a typical software project.

How do they differ?

They're actually good.

Though the size and risk of a project do materially matter. If chrome ships a bug, that'll cause an impact on billions of people potentially, where as the typical software bug will impact a few hundred or dozens of people?

The way the code quality of webkit built up was by introducing a review gate. Chrome inherited its review gate from webkit, because it had established a much better track record for code quality.

The way you get any project to be "actually good" is by adopting methods to ratchet quality. Early WebKit did not have the same strict rules it has today, and we suffered because of that. Introducing review + test for every change rule was introduced, and the frequency of regressions dropped dramatically.

> demo/quick mob session Are you suggesting that a meeting to review cost is going to be faster than a code review? And any comments will be lost?

And pair programming? No thanks.

I think it you are doing actual code reviews, you are doing something wrong.

Code reviews are incredibly valuable, and are a crucial part of software development at all of tech companies. I would argue that if you aren't doing code review, then you're saying that you believe you and all your company's engineers are better than the engineers at Google, Apple, Mozilla, Microsoft, Oracle, ....

Having engineers better than the average employee at those companies is a lower bar than you'd imagine.

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".

No, it isn't. I know the standard of engineers at FAANGs.

Having engineers better than the competent engineers that are responsible for the dreaded process at the FAANGs, MS, etc is hard. The review policies of big projects tend to have been hard learned, and dismissing them because "they're useless process" is a poor choice.

I'm speaking based on the experience of dozens of engineers, including ones from all of MFAANG, and a half dozen almost-FAANGs.

I think you're conflating whether a process is useful for the organization with whether it is useful for the engineers implementing it. Since processes have to be uniform, you need to either figure out how to retain someone that's been at the company for a decade to continue to work maintenance jobs, or you need to figure out how to keep the fresh grads that will replace them from breaking the world.

The right choice for the organization is to keep production stable, and the bus factor high.

The right choice for the engineer is to demand massive comp increases (comparable to startup acquisition windfalls), and also the abilty to stop working maintenance.

A lot of the best engineers I've worked with used to be in a FAANG. Almost none of them would consider going back. They universally cite broken processes as the reason they left.

I'm confused by this discussion about development speed: are you talking latency or throughput?

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.

> If you need to gate keep your team members then I’d question the strength of your team.

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.

> PR’s are great for open source projects as act as gatekeeper so not everyone can commit freely

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.

> 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

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.

> Sometimes junior, or less skilled people, have something valuable to say. Especially if the code could be simpler.

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.

I've never thought of a PR as causing conflict. For sure, you're right now that you've explained it, but as an engineer I've never felt that way.

But I'm okay with the conflict! There should be conflict at work! Ideas should be freely expressed and those ideas are going to meet contrary ones!

What wouldn't be healthy is a place where that conflict isn't resolved or doesn't lead to a better idea winning. Or where only the Lead "wins" because of their position.

There shouldn't be arguments, no one should yell or be hurt. For sure that's a bad place to work. But conflict about where to place a piece of code? Sure! Conflict about if we name it Foo or Bar? Why not?! That conflict is like the sharpening of Iron! It hurts _today_ but can strengthen and make _you_ better let alone the organization as a whole.

>But conflict about where to place a piece of code? Sure! Conflict about if we name it Foo or Bar? Why not?! That conflict is like the sharpening of Iron!

I dearly hope this is sarcasm. This is about the same level of absurdity as developers taking ages to pick project/file names. I'm not railing against a review's abilities to find bugs and make sure someone else understands. GP is right in pointing out how many fruitless review discussions exist over personal differences in what to call a function name because "I think X sounds better than Y", despite every party involved understanding the code and what it does.

If only linters could solve these trivialities.

I’m sorry you had such bad experience but you were working with the wrong people (feels like toxic even). PR reviews work best in environment where trust is key, and deciding together (emphasis on together) best approach to reach a goal is the main drive. In the end PRs should make you and the team stronger, as the knowledge is shared collectively.

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.

> I’m sorry you had such bad experience but you were working with the wrong people (feels like toxic even).

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.

It doesn’t have to be like this. Even when I was CTO, I’d submit my code in a PR and my team would pull me up on mistakes and inconsistencies. It was really annoying! And also great.

PRs are a great way for the whole team to learn about how the organisation cuts code, and can reduce the number of errors, but of course with poor leadership they can be used for evil.

Same here! I would have one of the more senior people review my code, and I would review his. It was a small organization and we didn't have time to do the most thorough reviews, but it was good that someone else knew something about what was going on.

"Code reviews", in general, are a good thing for knowledge transfer. If they are done for nit picking and stylistic complaints, they are not terribly valuable.

> It doesn’t have to be like this. Even when I was CTO, I’d submit my code in a PR and my team would pull me up on mistakes and inconsistencies. It was really annoying! And also great.

I can imagine that you had fun mingling with the commoners for a day, now try it on all of your actual work, and with the whole C-level team gang up on you for each review.

> about how the organisation cuts code

You don't have any guidelines for how the organisation cuts code, and that's why you like the review process, because it covers up for that.

Why the hate? You’ve obviously had some shitty experiences but I’ve never done anything to deserve your attitude.

> I can imagine that you had fun mingling with the commoners for a day, now try it on all of your actual work, and with the whole C-level team gang up on you for each review.

You literally know nothing about me, my experiences or how I conduct my life, and rather than listen to people with a different experience to yours, you choose borderline abuse.

This is the comments section of HN. Take a break and get some perspective.

I agree with the others commenting here and can't relate to your experience at all. Review is a huge positive and while it does "slow things down" it usually is preventing people from slamming into walls at high speed, so that's a positive too.

Maybe your work experience is mostly in a high stress, prototype-heavy environment where it's more important to launch something than it is to have a maintainable, incrementally improving codebase? I worked at a consultancy like that and it was very different from "big product" long term work.

As for the social dynamics, it sounds like your workplace culture just blows.

I've worked on several teams that required PRs before merging, and there was never any toxic behavior. Everybody was happy with it.

I’m not sure why people are downvoting your lived experience. I’ll just say one thing: quit. Right now. This is a BAD environment.

He's getting downvotes because he's persistently overgeneralizing from his unfortunate, legitimate lived experience to a bunch of dogmatic claims about the fundamental nature of the pull requests that contradict many other people's own lived experience.

I'm simply pointing out that the workflow of the pull request is made for a different workflow than what you normally have inside a team in a company, and it therefor quite a bad fit. And illustrating this with a few examples.

I'm getting downvoted because I'm criticising developers favorite tools that lets them pretend to be Linus Thorvalds for a moment.

No it isn't, though. It's not like there's a "right way" to run a team. The teams I've been on that didn't review code always ended up imploding because because people rationalize their innocent corner-cutting when they don't have to deal with the embarrassment of sending it off to anyone.

Reply on news.ycombinator.com