Unified versus Split Diff
matklad.github.io
Unified versus Split Diff
1–10 of 184 posts
Re: Unified versus Split Diff
#2> 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,
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/squashed it is relatively easy to review incremental changes in isolation anyway.
I guess the complaint author has is really about review types. 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.
Re: Unified versus Split Diff
#3> 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, 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 cod…
> 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.
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.
A good code review starts with a good PR: one that outlines what its goals were and how it achieved them.
First item on a good review then: does the code achieve what it set out to do per the outline?
Second: does it contain tests that validate the claimed functionality?
Third: does it respect the architectural conventions of the system so far?
Fourth: is the style in line with expectations?
Fifth, and finally: do you have any suggestions as to better API usage?
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.
An implicit question in several of the above is "will this set a good example for future contributions?"
Re: Unified versus Split Diff
#4> 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, 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 cod…
> 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. 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. A good code review starts with a good PR: one that outlines what its goa…
As a counterpoint, individual code ownership can be a fantastic model too. It lets engineers specialize and takes advantage of social systems that form naturally anyways. I’ve not personally seen group ownership work well, and in practice, it’s still an individual who knows a given area the best.
Re: Unified versus Split Diff
#5I haven't thought about this in detail much before but honestly that "view" the author proposed does look pretty nice.
Though I think that something like difftastic is still a bit better, though I haven't properly integrated it into my workflow.
Re: Unified versus Split Diff
#6I do the same! I checkout the branch of the PR then do a mixed reset to origin/main. This way, the git status shows all the changes. It’s really neat indeed.
Re: Unified versus Split Diff
#7I'm probably missing something here: author says that the split diff doesn't work for him, but doesn't say why.
His ideal diff is pretty much the same as a split-diff but with redundant context removed (context is only on the left, not on both left and right). What utility does he get out of removing redundant context on the RHS of the pane?
Re: Unified versus Split Diff
#8> 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, 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 cod…
> 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. 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. A good code review starts with a good PR: one that outlines what its goa…
As I have said - every team is different :)
In response to your first point I guess things really depend on where you store PR metadata and how ephemeral/permanent it is. Some teams store that information in the ticket, some fill implementation notes with change request, some add that to the PR, some discuss in their standup (or similar) meeting.
Regarding 2-3, you are right, I just lumped them all under umbrella term.
Maybe we could use terms like "PR review" and "code review". The former is a shallow LGTM check, while the latter may involve code checkout, poking around, architectural discussions, pair/team programming and so on. In my book they are entirely different beasts and web tools are geared (not without justification) towards the former, where both types of diff should serve the purpose.
Re: Unified versus Split Diff
#9Meld works pretty well for these use cases.
Re: Unified versus Split Diff
#10I think it's kind of funny that the left view is Word track changes.
I do think it's better actually, never thought about it. It might even function better if you could explode the changes out by clicking.