16 ms·

> Bonus points for no PR’s and trunk driven development as that shows a very mature team. Ugh, pass. Trunk development is fine. Skipping PRs just brings back n

• BFLpL0QNek 5y ago
  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.
• wildpeaks 5y ago
  I agree, the PR is invaluable to preserve context: the trunk history remains simple and linear thanks to squash merges, and you can still see the fine grained commits (and matching discussions/reviews) in the closed PR. It's also a way to give insight when onboarding new developers: if something is surprising, they can see why things came to be this way, not just accept the result at face value.
  • someweirdperson 5y ago
    Explanations should end up in comments, documents, and the commit-comment of the squash (creating a good one takes some time when squashing). The PR and original branch show dead-ends that should not be required reading to understand things.
    • shawnz 5y ago
      Maybe they should not be required in the ideal state, but I don't think it's wise to design your processes to only accommodate the ideal state.
    • NateEag 5y ago
      I'm personally a fan of small, clean commits, rebasing without squashing before merge, always making a merge commit, and writing a clear, through merge commit explaining what the branch does. That way, I can treat the series of merge commits to trunk as the simple, linear overview of history, but when I'm bug-hunting I get small, clear commits to search through with git bisect. It also means I get more useful blame output, as instead of huge hunks of a file being all from one giant bomb commit, I can see the process and evolution that the whole change went through as the original author pieced it together. That can be really helpful when dealing with obscure bugs or understanding systems with little documentation, by helping you get back more of the original author's thought process and seeing how their mental model evolved as they built the feature.
      • ithkuil 5y ago
        Do you ensure your clean commits all pass all CI tests?
• intel_brain 5y ago
  i think that was sarcastic.
• boomlinde 5y ago
  Where I work, development is trunk based and without pull requests, and all code is reviewed. When I want to submit code, I push it to a staging area that tracks master. This causes the commits to appear in Gerrit where (conflict-free) rebasing can be performed with a single button and the code can be reviewed. During the staging I can change anything about the commits to my hearts content. Once everyone is satisfied, someone with the authority to approve the change approves it and I submit it, upon which the patches are applied as-is on the branch I staged it for. Creating a separate branch, pushing this to your public copy of the repository and then asking someone to pull from that branch into their master branch seems absurd to me, especially if it's just 1-2 commits, and the idea of reviewing code (which I think is extremely important at team scale) should not be conflated with the concept of pull requests.
  • andrewflnr 5y ago
    That process sounds isomorphic to a PR.
  • doix 5y ago
    What you are describing, is pretty much exactly how it works at every company I've worked at that used PRs. You create a new branch from master, make your changes, push the branch to github/bitbucket/gitlab, make a PR. While the PR is open, you can make any changes you want to the commits in that branch (since it's just a branch). People look at the PR (which shows the diff) and approve it if they are happy, or request changes. If there are no conflicts, you can merge with a single button. Otherwise you need to resolve conflicts somehow. I normally just rebase the branch off updated master and force push. > Creating a separate branch, pushing this to your public copy of the repository and then asking someone to pull from that branch into their master branch seems absurd to me. That's how it works for open source projects because people do not have permissions to make branches in the main repo, so they must fork and have the changes in a different repo. I've never seen this done at a company, I presume everyone here is talking about creating branches in the main repo and requesting review before merging the branch to master.
    • kibwen 5y ago
• auspex 5y ago
  Workflow and pull request approvals are required for SOC 2 type1/2 compliance.
• jjfoooo6 5y ago
  It really seems like this "no PR" idea is having a moment on HN. I can't tell if it's just engagement bait contrarianism or genuine. That any developer would trust themselves even to just not make simple typo's is baffling to me. Maybe the kernel of a good idea in the comment: A team that prioritizes reviews, such that PR's are not a significant hurdle, is indeed a sign of a mature team.