<story><title>Unified versus Split Diff</title><url>https://matklad.github.io/2023/10/23/unified-vs-split-diff.html</url></story><parent_chain><item><author>crdrost</author><text>I go back and forth on this.&lt;p&gt;On the one hand, I really like constant deep feedback. I really like the consistency benefits of having another person say “that’s too much, I find that unreadable.”&lt;p&gt;On the other, I have now been at a lot of places where it was very hard to get my code reviewed, latencies of days and sometimes weeks if folks are in a particularly heinous crunchtime... And then when it does get reviewed, the stuff that I worked really hard on to get the right speed or to properly centralize cache invalidation... Suddenly someone is like “I would have done it from this other approach” and you have no idea whether it&amp;#x27;s tractable or not.&lt;p&gt;While I have never been at a place that did this, I have in my head the idea that the code should be an unfolding collective conversation, kind of like when folks are all collaborating on a shared Google Doc, I see that you are editing this section and I throw in a quick comment “don&amp;#x27;t forget to add XYZ” and then jump to a different part that I won&amp;#x27;t be stepping on their toes with. So the basic idea would be to get everybody to merge to `main` like 2 or 3 times a day if possible. In that case code review really is just “make sure this doesn&amp;#x27;t break the build or break prod, everything is behind a feature toggle, if I don&amp;#x27;t like the design I will comment on the code near the design or sketch a proof of concept showing that my approach is superior in clarity or speed or whatever”... Nobody ever takes me up on this culture shift it seems.</text></item><item><author>dylukes</author><text>&amp;gt; In my book a general code review is simple sanity check by a second pair of eyes, which can result in suggestions to use different API or use an API slightly differently.&lt;p&gt;This is an impoverished view of code review. Code review is a principal mechanism for reducing individual code ownership, for propagating conventions, and for skill transfer.&lt;p&gt;A good code review starts with a good PR: one that outlines what its goals were and how it achieved them.&lt;p&gt;First item on a good review then: does the code achieve what it set out to do per the outline?&lt;p&gt;Second: does it contain tests that validate the claimed functionality?&lt;p&gt;Third: does it respect the architectural conventions of the system so far?&lt;p&gt;Fourth: is the style in line with expectations?&lt;p&gt;Fifth, and finally: do you have any suggestions as to better API usage?&lt;p&gt;A code review that is nothing more than a sanity check is useless and could have been done by CI infrastructure. Code review is a human process and should maximally take advantage of the things only humans could do. Leave the rest to machines.&lt;p&gt;An implicit question in several of the above is &amp;quot;will this set a good example for future contributions?&amp;quot;</text></item><item><author>friendzis</author><text>&amp;gt; For a large change, I don’t want to do a “diff review”, I want to do a proper code review of a codebase at a particular instant in time, paying specific attention to the recently changed areas,&lt;p&gt;obviously every team and ticket is different, but IMO unless the person doing the review is some sort of principal engineer mostly responsible for the code at large, this does not align with I would personally consider a code review. In my book a general code review is simple sanity check by a second pair of eyes, which can result in suggestions to use different API or use an API slightly differently. If commits in the PR are properly ordered&amp;#x2F;squashed it is relatively easy to review incremental changes in isolation anyway.&lt;p&gt;I guess the complaint author has is really about review &lt;i&gt;types&lt;/i&gt;. I do not have terminology ready at hand, but there is run of the mill sanity check review and then there is deep, architectural feature premerge review. The author complains about the latter when most of the code reviews in web tools are of the former variety.</text></item></parent_chain><comment><author>jaggederest</author><text>In this kind of context, I ask people what log level they&amp;#x27;d like their review at. If you just want to get the code out the door, by all means, &amp;quot;error&amp;quot; or &amp;quot;warn&amp;quot; might be the right review depth, when you&amp;#x27;re confident in your code and don&amp;#x27;t want to be derailed with philosophy.&lt;p&gt;If you&amp;#x27;re exploring a new concept and want all the ideas and brainstorming you can get in your feedback, &amp;quot;debug&amp;quot; log level is appropriate.&lt;p&gt;Once that idea has moved down the pipe, you may be down to &amp;quot;info&amp;quot; or &amp;quot;warn&amp;quot; depending on how much conversation has happened around the PR.</text></comment>
<story><title>Unified versus Split Diff</title><url>https://matklad.github.io/2023/10/23/unified-vs-split-diff.html</url></story><parent_chain><item><author>crdrost</author><text>I go back and forth on this.&lt;p&gt;On the one hand, I really like constant deep feedback. I really like the consistency benefits of having another person say “that’s too much, I find that unreadable.”&lt;p&gt;On the other, I have now been at a lot of places where it was very hard to get my code reviewed, latencies of days and sometimes weeks if folks are in a particularly heinous crunchtime... And then when it does get reviewed, the stuff that I worked really hard on to get the right speed or to properly centralize cache invalidation... Suddenly someone is like “I would have done it from this other approach” and you have no idea whether it&amp;#x27;s tractable or not.&lt;p&gt;While I have never been at a place that did this, I have in my head the idea that the code should be an unfolding collective conversation, kind of like when folks are all collaborating on a shared Google Doc, I see that you are editing this section and I throw in a quick comment “don&amp;#x27;t forget to add XYZ” and then jump to a different part that I won&amp;#x27;t be stepping on their toes with. So the basic idea would be to get everybody to merge to `main` like 2 or 3 times a day if possible. In that case code review really is just “make sure this doesn&amp;#x27;t break the build or break prod, everything is behind a feature toggle, if I don&amp;#x27;t like the design I will comment on the code near the design or sketch a proof of concept showing that my approach is superior in clarity or speed or whatever”... Nobody ever takes me up on this culture shift it seems.</text></item><item><author>dylukes</author><text>&amp;gt; In my book a general code review is simple sanity check by a second pair of eyes, which can result in suggestions to use different API or use an API slightly differently.&lt;p&gt;This is an impoverished view of code review. Code review is a principal mechanism for reducing individual code ownership, for propagating conventions, and for skill transfer.&lt;p&gt;A good code review starts with a good PR: one that outlines what its goals were and how it achieved them.&lt;p&gt;First item on a good review then: does the code achieve what it set out to do per the outline?&lt;p&gt;Second: does it contain tests that validate the claimed functionality?&lt;p&gt;Third: does it respect the architectural conventions of the system so far?&lt;p&gt;Fourth: is the style in line with expectations?&lt;p&gt;Fifth, and finally: do you have any suggestions as to better API usage?&lt;p&gt;A code review that is nothing more than a sanity check is useless and could have been done by CI infrastructure. Code review is a human process and should maximally take advantage of the things only humans could do. Leave the rest to machines.&lt;p&gt;An implicit question in several of the above is &amp;quot;will this set a good example for future contributions?&amp;quot;</text></item><item><author>friendzis</author><text>&amp;gt; For a large change, I don’t want to do a “diff review”, I want to do a proper code review of a codebase at a particular instant in time, paying specific attention to the recently changed areas,&lt;p&gt;obviously every team and ticket is different, but IMO unless the person doing the review is some sort of principal engineer mostly responsible for the code at large, this does not align with I would personally consider a code review. In my book a general code review is simple sanity check by a second pair of eyes, which can result in suggestions to use different API or use an API slightly differently. If commits in the PR are properly ordered&amp;#x2F;squashed it is relatively easy to review incremental changes in isolation anyway.&lt;p&gt;I guess the complaint author has is really about review &lt;i&gt;types&lt;/i&gt;. I do not have terminology ready at hand, but there is run of the mill sanity check review and then there is deep, architectural feature premerge review. The author complains about the latter when most of the code reviews in web tools are of the former variety.</text></item></parent_chain><comment><author>bernds74</author><text>&amp;quot;I would have done it from this other approach&amp;quot;. I&amp;#x27;ve seen that, and it&amp;#x27;s not good when you get the feeling of &amp;quot;code review is when someone who hasn&amp;#x27;t thought about your problem tells you how you should have solved it&amp;quot;. People sometimes feel they have to add value as a reviewer, and casually discarding other people&amp;#x27;s work is the way to do it. Fortunately it&amp;#x27;s not something I have to deal with at my current job.</text></comment>