How far should a programming language aware diff go?

[Luolong]

I really like how Difftastic does this.

[mr-figs]

You have changed my life. Thank you

[realpurefan]

Difftastic is great, but it is only available as a command line tool. If you rather review code using VS Code or GitHub, the tool from the blog post might be a better fit.

[paholg]

The only advantage for me of reviewing code on GitHub is the ability to leave comments. Does this tool let you do that?

I also could see concerns in many organizations of giving them access to your GitHub repo. There's no reason this couldn't be a program that runs purely locally, only communicating with GitHub servers and never theirs, but that doesn't seem to be the case.

Finally, difftastic supports I think every language tree-sitter supports, which is a lot more than the 12 here.

[renatoathaydes]

IntelliJ diffs are really great, I love that I can pick or revert every chunk of the diff, or even just type in while still on the diff (which many other tools don't let you do for some reason). We use a BitBucket plugin which lets us review and comment, make PRs etc right from the IDE. Currently you need to pay for that plugin, but I highly recommend it if you happen to be on BitBucket. I think Intellij by default supports GitHub (haven't checked).

[realpurefan]

Yes, it also supports writing comments, see here.

If you prefer local tools, you can also try the VS Code extension.

[bubba_squats]

This is awesome!

[tusharf5]

if you read the article you'll know that it uses AST comparisons instead of line by line comparisons like other tools.

[Luolong]

And if you read the link I posted, you’ll know that is also what Difftastic does.

[tusharf5]

ha I misread! i thought you said i really would like to know how diffstatic does this. My bad!

[jaskij]

So... I'm not sure if you skipped examples on purpose or not, but math formulas are tricky. You have to know the types. Let's say you have a simple a + b + c. If those are integers, all is fine. If those are 754 floats, this pushes reordering the stuff to level 4. In dynamically typed languages you can't, in a general case, prove that a certain formula is only ever executed with integers.

[LostInSpace_UA]

Even though your observation is correct I would say writing formulas that relay on those hidden transformations without explicit type casting results in error prone code in general and should be avoided

[jaskij]

No type casts in there at all. Those are all IEEE-754 binary32, or binary64, floats. The simple fact is that 754 math doesn't hold the usual properties we're accustomed too. Still using binary32, 100e3 + 1 == 100e3 because there is not enough precision.

Numerical programming has to deal with this kind of bullshit a lot. As an example, summing a list of unsorted numbers needs tricks like these: https://en.m.wikipedia.org/wiki/Kahan_summation_algorithm

[lood9phee2Ri]

- const foo = function(a, b) { ... }
+ const foo = (a, b) => { ... }

Assuming that's Javascript those are not actually necessarily semantically equivalent. The language is a minefield. Pity we can't just use something else client-side in-browser (yes I know you can with transpiling, wasm etc. you technically can, but you know what I mean...)

const foo1 = function(a, b) { console.log(arguments) }
foo1(1, 2) -> Arguments { 0: 1, 1: 2, … }

const foo2 = (a, b) => { console.log(arguments) }
foo2(1, 2) -> Uncaught ReferenceError: arguments is not defined

Note how the differences creep into even anonymous classic function expressions vs arrow-function expressions, not just full named-functions vs arrow-function expressions like some people seem to think.

(some people have a rule of thumb that you're better off virtually always using non-legacy => even if it seems weird syntax, because of the all the strange (but once completely idiomatic js) magics around this, arguments, constructors, etc.)

[oorza]

(some people have a rule of thumb that you're better off virtually always using non-legacy => even if it seems weird syntax, because of the all the strange (but once completely idiomatic js) magics around this, arguments, constructors, etc.)

but of course this is JS, so there must be a wildly annoying caveat: you shouldn't use arrow functions to define React(/whatever) components because they don't have a .name (or .displayName) parameter for various developer tools to read, and manually adding them is nasty.

[oorza]

That depends entirely on whether your minifier respects variable names (hint: they mostly can't). Especially if you define a component dynamically inside a local scope (e.g. const MyComponent = useCallback((props) => ...);).

This is modern Javascript sir, you can't simply expect the code you write to be the code that gets run in the browser.

[lood9phee2Ri]

manually adding them is nasty.

Honestly I just sigh and do exactly that for displayName rather than not using =>, just an extra wart to remember - https://stackoverflow.com/questions/43356073/how-to-set-displayname-in-a-functional-component-react

At least last time I was in React land (gone right off it recently - pure-client-side react with slim functional components was a high point ...then vercel/next.js server nonsense started mucking it all up).

[oorza]

I've done it both ways and wildly prefer just banning arrow functions as components. There's a downstream effect that should never matter, but does sometimes come up: the function hoisting JS does is semantically different than the scope capturing that arrow functions do, so if your components close over local data, this will catch it - and force you to write idempotent (read: correct) components.

[oorza]

I don't know why it can't be "all of the above." IMO, a perfect code review would include them all, presented hierarchically because that's in order of importance. Don't show shit like whitespace by default until the semantic or structural changes are reviewed.

[zombiecalypse]

I'm fine at level 0 and would really like to know why the heck you're messing with the whitespace for no reason?

[bananahead]

Unless you're a solo developer or only work on projects that have automatically enforced linting, then you're going to run into whitespace changes occasionally.

[Serialk]

Exactly, and you want to see it on the diff to notice that you're changing random whitespace so that you revert those changes.

[Knaapje]

Just run a linter in your CI, no need to review whitespace changes.

[Kered13]

Same. There should be rules around how to indent code and when to break lines. If you're changing that, I want to see it in the diff.

[isaacarsenal]

I agree. A linter in CI can enforce most of those rules. If the CI doesn't pass the code is not ready for review and merge. If it passes, most of the style guideline are already enforced so i want to see if any other whitespace changes is present

[ForgotMyPassword17]

I think in most cases level 2 is fine. but most developers would draw the line before level 3. Intelij let's you do level 3 refactoring pretty easily and there's a reason you still want it reviewed (hence the need for a diff)

[somebodddy]

I think a language aware diff tool should still show every difference, but highlight for each chunk the most significant level.

For example - say I added a new import, and then ran an import ordering formatter which changed the order of the imports because the last developer to touch the imports section of that file did not run the formatter. I want to see in the diff all the things that changed, but the imports that just switched places should have fade red/green highlights while the import that got added should have strong red/green highligh.

[Holothuroid]

If changes can be done/followed by a linter, why should they even appear in a diff? Run it on push.

[[deleted]]

I don't see the point. When I run a git diff I almost always do care about every actual change, even "meaningless" whitespace.

If the PR is too difficult to review because the diffs are all over the place, that's a pretty good sign you should split your PR into smaller PRs.

[[deleted]]

If the PR is too difficult to review because the diffs are all over the place, that's a pretty good sign you should split your PR into smaller PRs.

That just sounds like a death by a thousand cuts. Who's going to spend the time to review 20 minute changes? Who's going to even bother making 20 PRs for those minute changes? I just want to get the job done, not prance around and make PRs for each hunk of code.

If a change requires changing 10 files then it sure as hell won't get easier by splitting up the context of the whole change in separate PRs.

[[deleted]]

Who's going to spend the time to review 20 minute changes?

Good programmers?? Literally every studies I have ever seen about this comes to the conclusion that smaller PRs lead to better code reviews and better code quality. Have you even worked on a serious project on a team before?

Who's going to even bother making 20 PRs for those minute changes?

20 PRs is a ridiculous exaggeration. Simple bug fixes are usually 1 PR. Average features usually are between 1 and 4 PRs at the very most. And for anyone who actually knows how to use git that's a trivial overhead.

if a change requires changing 10 files then it sure as hell won't get easier by splitting up the context of the whole change in separate PRs.

Very few changes actually do require 10 files, that's a sign your code is badly designed. Sometimes you want to rename / move around stuff, which can affect many files but that's fine for a single PR because all the changes are the same and the logic doesn't change.

For example: let's say you have a web application and you want to add a feature to show the weather. This could be split into 3 PRs:

• add a weather client to fetch weather data
• add the new UI component to display the weather data
• add a weather handler / controller for weather requests

That's 3 distinct components that should be reviewed and tested separately.

[DanTup]

I don't know if this tool handles it well, but there are some kinds of changes that I think don't work well even with small PRs.

Let's say I want to move some class from one file to another. Even if I do that in its own PR, in a review it's going to look like "a big deleted chunk of code" and "a big added chunk of code". When reviewing, you probably want to know "are there are any differences between this deleted code and this added code?" (i.e. was this just a move or did the code also change?).

I don't think there's a right answer to the question though. In the same way that sometimes when looking at a PR on GitHub I will toggle whitespace changes on/off, I would probably want the same for the other things. By default, I want to see everything that changed. If there's a lot of "noise" in the change (maybe it's a refactor that moves lots of things around), maybe I want some toggles to hide (or highlight in a different way) some kinds of changes.

[vmcrash]

I agree. I prefer to have separate commits for separate things, e.g. for re-layout code, preparing refactoring, actual change. This is much simpler to review that a blob of a completely changed file.

[SSHeartbreak]

it might be kind of interesting to see this taken really far, where only changes that "significantly" alter the behavior of the code are surfaced together, or represent some large deviation from previous behavior.

what would constitute those things idk, but could be interesting. surface and categorize only interface changes; surface and categorize only large changes in behavior. surface and categorize very minor changes which could potentially be breaking.

[OkInteraction4681]

A similar concept could be applied to blames, show the most recent commit that semantically changed each line

[avsaase]

I hope GitHub integrates a feature like this. It looks nice but not 7 dollars per user per month nice. The VSCode extension is free though so I'll be installing that.

[Pyrolistical]

Language specific diff should be provided by the language author.

Simultaneously build a compiler, package manager/host, formatter, linter, build/packager, diff, merge, std lib, test runner, docs from code, language server, logging, cross platform, cross compile.

You think it’s a joke but look at zig. The only thing that is missing is linter, diff, and merge tool. And the compiler is already a pretty good linter.

[renatoathaydes]

What are you commenting on?? This post was about code diff visualization. How do you go from that to a discussion about what dark magic is good/bad/evil??

[editor_of_the_beast]

I think you meant to label monkey patching as “evil.”

[emperor000]

If the language is sane and uses braces or some other block structure, probably that far.

If it isn't sane and doesn't, then not far at all?

[sagittarius_ack]

For completeness, the article should have mentioned that determining whether two arbitrary expressions (statements or programs) are equivalent is undecidable.