8 ms·

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

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.

deleted 5y ago
[deleted]
chrismcb 5y ago
> 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.
- olliej 5y ago
  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, ....
  - hedora 5y ago
    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".
    - olliej 5y ago
      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.
      - hedora 5y ago
        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.
anyfoo 5y ago
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.
- rileyphone 5y ago
  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.
  - olliej 5y ago
    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.
    - dhzhzjsbevs 5y ago
hamburglar 5y ago
> 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.
olliej 5y ago
> 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??
- turminal 5y ago
  The projects listed bear very little resemblance to a typical software project.
  - olliej 5y ago
    How do they differ?
    - beckingz 5y ago
      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?
      - olliej 5y ago
        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.
onion2k 5y ago
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.
- hedora 5y ago
  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.)
hinkley 5y ago
> 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.
- doctor_eval 5y ago
  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.
ozim 5y ago
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.
- the_gipsy 5y ago
  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.
  - ozim 5y ago
    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.
    - the_gipsy 5y ago
      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.
      - ozim 5y ago
        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".
jseban 5y ago
> 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.
- aaronbrethorst 5y ago
  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.
  - NateEag 5y ago
    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.
    - jseban 5y ago
      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.
      - 5y ago
golergka 5y ago
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.

deleted 5y ago

[deleted]

chrismcb 5y ago

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

olliej 5y ago
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, ....
- hedora 5y ago
  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".
  - olliej 5y ago
    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.
    - hedora 5y ago
      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.

anyfoo 5y ago

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.

rileyphone 5y ago
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.
- olliej 5y ago
  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.
  - dhzhzjsbevs 5y ago

hamburglar 5y ago

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

olliej 5y ago

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

turminal 5y ago
The projects listed bear very little resemblance to a typical software project.
- olliej 5y ago
  How do they differ?
  - beckingz 5y ago
    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?
    - olliej 5y ago
      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.

onion2k 5y ago

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.

hedora 5y ago
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.)

hinkley 5y ago

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

doctor_eval 5y ago
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.

ozim 5y ago

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.

the_gipsy 5y ago
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.
- ozim 5y ago
  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.
  - the_gipsy 5y ago
    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.
    - ozim 5y ago
      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".

jseban 5y ago

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

aaronbrethorst 5y ago
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.
- NateEag 5y ago
  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.
  - jseban 5y ago
    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.
    - 5y ago

golergka 5y ago

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.