Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Unified versus Split Diff

matklad.github.io

Post reply on HN
Print view View original

71–80 of 184 posts

Re: Unified versus Split Diff

#71

Post by jaggederest »

crdrost wrote: post #29
dylukes wrote: post #3

Earlier quoted context omitted.

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

I go back and forth on this. 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.” 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, t…

In this kind of context, I ask people what log level they'd like their review at. If you just want to get the code out the door, by all means, "error" or "warn" might be the right review depth, when you're confident in your code and don't want to be derailed with philosophy.

If you're exploring a new concept and want all the ideas and brainstorming you can get in your feedback, "debug" log level is appropriate.

Once that idea has moved down the pipe, you may be down to "info" or "warn" depending on how much conversation has happened around the PR.

Re: Unified versus Split Diff

#72

Post by lapcat »

eru wrote: post #62
lapcat wrote: post #57

Earlier quoted context omitted.

In my book, superficial code review produces only superficial results. This is how so many bugs get shipped despite the ubiquity of code review. Call me crazy, but I've always approached code review from the standpoint of a tester: I build and run the code, see whether it does what it's supposed to do, look for possible weak points, and try to break it. The diffs are the beginning rather than the end. They show you w…

Automated tests aren't so much to make sure that the test you just wrote works. You are right that manual testing can do that better sometimes. Automated tests are so that the code you just implemented still works next months, when lots of other people have made unrelated changes, and weren't always aware of the interactions with other parts of the code. The automation is important, so that the tests get run, even wh…

To be clear, I'm not against automation. What we do for a living is write code, so when I say "automation is just more possibly buggy code", I'm not opposed writing more code (though I am opposed to overengineering). My point is that I don't place all of my faith in automated tests, and I'm not opposed to rolling up my sleeves, getting my hands dirty, and doing manual labor. Manual labor can be tedious, though, and that's probably why a lot of engineers hate it.

Re: Unified versus Split Diff

#73

Post by williamcotton »

koolba wrote: post #56
crdrost wrote: post #29

Earlier quoted context omitted.

I go back and forth on this. 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.” 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, t…

> 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't forget to add XYZ” and then jump to a different part that I won't be stepping on their toes with. You just discovered pair programm…

With the right pair it is unbelievable how much productivity can be unleashed. I concur, it's much easier to stay focused on the task at hand with someone actively working on the same thing.

The reason why this didn't catch on is that it's almost like a Mick and Keith sort of relationship. You can't just take any two musicians and throw them together and get the Rolling Stones and the same thing applies to pair programming.

Take a look at Bell Labs during the birth of UNIX for an entire SWARM of interdependent engineers. There's more than just a small element of good fortune! It seems those people were almost meant for one another.

Re: Unified versus Split Diff

#74

Post by mannykannot »

dylukes wrote: post #3
friendzis wrote: post #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 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…

This is an ideal (if not utopian) vision of software review (though please let's handle style checking automatically already!) It implicitly requires, however, such a thorough examination of the code that diffs would seem to be an irrelevant distraction.

In practice, however, starting from the changes in a system that was previously working well enough is a very effective way of focusing limited human attention on where the problems are likely to be, optimizing for error detection with limited resources over the broader knowledge-dissemination goals espoused in this approach. If we are going to use diffs, then this brings us back around to the topic if the article.

Personally, I find having any form of diff embedded in what I am trying to understand just makes it harder to follow, so I move the diffs onto a secondary screen and use it as a guide and reference to what has been changed. The author of the article seems to want the same, but the mock-up is somewhat misleading, as it only has substitutions and additions. Where something is removed or refactored to another place, the two views will no longer line up as depicted here.

Re: Unified versus Split Diff

#75

Post by jakub_g »

inferiorhuman wrote: post #69
jakub_g wrote: post #31

FYI if using GitHub, on github.com pull request page, press `.`, or change domain name to github.dev. This will open VSCode in browser, with the pull request in diff view. Advantages: - you see whole files there - diff algo is different than on github.com, sometimes more readable for complex diffs

Disadvantages: * S-L-O-W * Glitchy Which is a travesty because a side by side diff view with an out-of-line list of PR comments could be incredibly useful if it didn't involve spinning up a whole VSCode instance in the browser. In fact it's so useful that GH used to have a split view available without forcing people into their buzzword AI ML crypto blockchain cloud enabled dev environment nonsense. Good idea, abysmal…

GH still has a split view available. Above the diff, you have a settings icon which shows dropdown where you can changed between unified and split diff.

Re: Unified versus Split Diff

#76

Post by ozim »

dylukes wrote: post #3
friendzis wrote: post #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 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…

Code style check should be automated. I do not accept discussions on style in PR in my team besides pointing out obvious deviations which should be automated. Discussion on that should be way before making PR.

Architectural changes/discussion should be discussed by developers way before PR on slack or in a call. Most features should not change architecture and team should make effort to align architecture all the time at least before someone makes PR. Unless of course PR is PoC to showcase approach.

Re: Unified versus Split Diff

#77

Post by mannykannot »

phillipcarter wrote: post #4
dylukes wrote: post #3

Earlier quoted context omitted.

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

I feel the post you are replying to is really advocating for shared knowledge, which is entirely compatible with the positive aspects of ownership. Personally, I have seen nothing but trouble from people whose distorted concept of ownership opposes sharing knowledge of "their" stuff.

Re: Unified versus Split Diff

#78

Post by grilledcheez »

> I need to run tests, use goto definition and other editor navigation features, apply local changes to check if some things could have been written differently, look at the wider context to notice things that should have been changed, and in general notice anything that might be not quite right with the codebase

It sounds like the author really wants pair programming

Re: Unified versus Split Diff

#79

Post by grotorea »

videlov wrote: post #48
knubie wrote: post #18

A third (fourth?) option worth mentioning here is difftastic[0], which uses "structural" diffing (as opposed to line diffing) for more granular diff highlighting. [0] https://github.com/Wilfred/difftastic

Thank you for sharing this project! I have been searching for something like this for some time

I also wonder if it's possible to go beyond this project and have git itself work on the syntax level instead of pure text.

Re: Unified versus Split Diff

#80

Post by btown »

grilledcheez wrote: post #78

> I need to run tests, use goto definition and other editor navigation features, apply local changes to check if some things could have been written differently, look at the wider context to notice things that should have been changed, and in general notice anything that might be not quite right with the codebase It sounds like the author really wants pair programming

There are certainly situations where, as a reviewer, 5 minutes of goto-definition on a PR branch, or a well-placed debug breakpoint while running a test, is all you need to write a more prescriptive comment than “I don’t understand how this data flows” - and thus avoid both a follow up meeting and the need to pair program in the first place.

Pairing absolutely has its place, but it’s not always the most optimal use of time.

Post reply on HN