4 ms·

Skipping code reviews for faster pushes to "trunk" does not sound like a stable solution. Why not just encourage faster reviews and smaller diffs..

geocar 9y ago
As explained in the article: Because experimentally, "smaller diffs" cause people to nitpick on pointless things like whitespace instead of performance, features, and future-proofing the architecture (i.e. the fucking point).
- aurelijus 9y ago
  This means code review process is completely wrong. All these "nitpicks" should be automated by linters and code review should be more about implementation, structure, performance, etc..
  - auggierose 9y ago
    But some people just cant help themselves. They are natural nitpickers and this process just enables them ...
  - geocar 9y ago
    Yes, it does mean that code review is completely wrong. If you read the article, they suggest strongly that it is hard to get a "correct" code review process (perhaps because it encourages the nitpickers, or perhaps for other reasons). If you've got a bunch of experienced people spending a year on it that can't solve it, perhaps they just can't solve it. And at that point: What's the difference between something that's wrong, and something that they can't do right?
  - watwut 9y ago
    Most review processes are wrong. Sometimes because it rubber stamp everything, other times because the most aggressive one is not the most knowledgeable one and yet other times cause they turn into competition of who is more petty. Good code review is hard. Catching non petty problems is much harder then lengthy obsessing over function names or 'if' vs '?' or whether slightly more or slightly less abstraction or whether two 6 line long function vs one 12 lines long.
- falsedan 9y ago
  That's not what they said: > Code review happens through a small window. When reviewing a PR you only look at the fraction of the code that just changed. Their complaint is that code review makes it easy to miss deviations from global goals & style, not that they nitpick minor presentation issues. Although I wonder what sftware they are using that would only show them a small fraction of a change…
  - Sean1708 9y ago
    > the fraction of the code that just changed. I.e. all of the code that changed, which is a fraction of the total codebase.
    - falsedan 9y ago
      Ah, thanks for clarifying.
- deleted 9y ago
  [deleted]
alkonaut 9y ago
> Skipping code reviews for faster pushes to "trunk" does not sound like a stable solution. I think a important context here is that this is engine code (a.k.a. library code). This isn't a web service or web site where what you ship is hitting end users. The end users of this code produce new products, and the potential issues are a) that the users of the engine (i.e. game studios, internal users of Stingray etc) are being held up in their work, or b) that bugs sneak through to their users (the gamers/end users). In this context, the idea of a quicker turnaround in exchange for some bugs leaking through is much easier to defend. The studio might want the buggy code faster rather than the fixed code later. This is probably not the case for a system deployed directly to end users. > Why not just encourage faster reviews and smaller diffs.. That was the motivation of switching to trunk based (i.e. removinbg the incentive and slowness of larger PR's). You can't "encourage" smaller diffs other than having the process inherently do that. It's not "encouraging" to send an email to the team telling them to make smaller diffs.

geocar 9y ago

As explained in the article: Because experimentally, "smaller diffs" cause people to nitpick on pointless things like whitespace instead of performance, features, and future-proofing the architecture (i.e. the fucking point).

aurelijus 9y ago
This means code review process is completely wrong. All these "nitpicks" should be automated by linters and code review should be more about implementation, structure, performance, etc..
- auggierose 9y ago
  But some people just cant help themselves. They are natural nitpickers and this process just enables them ...
- geocar 9y ago
  Yes, it does mean that code review is completely wrong. If you read the article, they suggest strongly that it is hard to get a "correct" code review process (perhaps because it encourages the nitpickers, or perhaps for other reasons). If you've got a bunch of experienced people spending a year on it that can't solve it, perhaps they just can't solve it. And at that point: What's the difference between something that's wrong, and something that they can't do right?
- watwut 9y ago
  Most review processes are wrong. Sometimes because it rubber stamp everything, other times because the most aggressive one is not the most knowledgeable one and yet other times cause they turn into competition of who is more petty. Good code review is hard. Catching non petty problems is much harder then lengthy obsessing over function names or 'if' vs '?' or whether slightly more or slightly less abstraction or whether two 6 line long function vs one 12 lines long.
falsedan 9y ago
That's not what they said: > Code review happens through a small window. When reviewing a PR you only look at the fraction of the code that just changed. Their complaint is that code review makes it easy to miss deviations from global goals & style, not that they nitpick minor presentation issues. Although I wonder what sftware they are using that would only show them a small fraction of a change…
- Sean1708 9y ago
  > the fraction of the code that just changed. I.e. all of the code that changed, which is a fraction of the total codebase.
  - falsedan 9y ago
    Ah, thanks for clarifying.
deleted 9y ago
[deleted]

alkonaut 9y ago

> Skipping code reviews for faster pushes to "trunk" does not sound like a stable solution. I think a important context here is that this is engine code (a.k.a. library code). This isn't a web service or web site where what you ship is hitting end users. The end users of this code produce new products, and the potential issues are a) that the users of the engine (i.e. game studios, internal users of Stingray etc) are being held up in their work, or b) that bugs sneak through to their users (the gamers/end users). In this context, the idea of a quicker turnaround in exchange for some bugs leaking through is much easier to defend. The studio might want the buggy code faster rather than the fixed code later. This is probably not the case for a system deployed directly to end users. > Why not just encourage faster reviews and smaller diffs.. That was the motivation of switching to trunk based (i.e. removinbg the incentive and slowness of larger PR's). You can't "encourage" smaller diffs other than having the process inherently do that. It's not "encouraging" to send an email to the team telling them to make smaller diffs.