[#46] Use similar to identify character-level diffs, then bold those changes

[nk9]

Tests still pass, but not sure if you'd like some other ones for this.

Note that this does add a dependency on similar.

[meta-cla[bot]]

Hi @nk9!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

[meta-cla[bot]]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

[nk9]

I want to request a code review from @swolchok. I tried to follow these instructions, but I don't see any Reviewers in the sidebar, so I'm not able to. Hope this will suffice!

[swolchok]

also, thanks, I can see the bolding in the tests and this looks nice! (this change causes output to be echoed to the terminal during cargo test, but we already weren't being very picky about that: we were emitting a bunch of blank space anyway.)

[nk9]

Thanks for the quick reply! I'll have a look tomorrow and see about addressing your requests.

[nk9]

OK, I think I've addressed everything. The terminal module is now just a wrapper for crossterm. Note that I've switched from execute! to print! in most cases because this plays nice with the automatic "hide stdout during tests unless there's an error" behavior, so cargo test now looks as it should.

I've also entirely removed use of diff. similar isn't quite as ergonomic as diff at the line level, but it's not too bad. Note that diff results now include a newline, so I've had to adjust the test cases around that. It may be possible to change this by adjusting newline_terminated, but I haven't played around with that.

I have only tested this with UTF-8 files containing newlines. Probably there should be some more tests with more challenging input, but LMK if you think that's required to merge this PR.

Oh, also: I had AI convert the diffs_to_print method to use Change. It seems to have mostly kept the logic as-is, and I made a few tweaks myself… but I'm not as confident in my understanding of the second half of the function, so please do take a look at that!

[facebook-github-bot]

@swolchok has imported this pull request. If you are a Meta employee, you can view this in D83374118.

[nk9]

I've discovered two issues with this code. First, when the change happens on the final line and the file doesn't end with a newline, you get this:

The other issue is that, unless the current change is on the last line, the last line of the file is not shown in the preview. I have a fix for both issues ready to go. Should I push another commit to this branch, or do you want a new PR?

[swolchok]

I've discovered two issues with this code. First, when the change happens on the final line and the file doesn't end with a newline, you get this:

The other issue is that, unless the current change is on the last line, the last line of the file is not shown in the preview. I have a fix for both issues ready to go. Should I push another commit to this branch, or do you want a new PR?

another commit is fine (please include regression tests); I can just re-import since we haven't landed yet.

[nk9]

I spent some time on this yesterday. First of all, here's the code so far. In addition to the two things I noticed before, I discovered another serious issue with the existing PR. I was checking specifically for a removal line followed by an insertion line, so the output is wrong if the regex involves mulitpile lines or if the replacement line is removed entirely.

similar has code to make this simple, which I've adopted in these new commits. This allowed me to remove a lot of code, as you'll see. However:

• Currently, it seems that the recommended simple inline diff approach only lets you show emphasized words, not the specific characters which have changed within those words. My original PR highlights characters, and I think that's what I'd prefer if possible. Are we on the same page there? I have opened an issue about this. I'm hoping there's an easy way to do character-level diffs which I haven't noticed yet.
• Instead of all the manual calculation in diffs_to_print, I've relied on similar's grouped_ops() function. The docs don't say, but the usize you pass appears to be the number of context lines above or below the change. So just passing half of the calculated terminal size seems to behave like the old code. But it isn't exactly the same, so I wanted to check if this is close enough.

You asked about tests, and I had two questions on that.

• Since the diffs_to_print function has been removed in favor of the similar equivalent, I've had to remove the two tests which rely on diffs_to_print (test_diff_with_unchanged_line_in_middle and test_diff_no_changes). No point testing the library's code in fastmod. Let me know your thoughts on this.
• You wanted me to write some tests for the new code, which seems like a good idea. However, I don't see an existing test which actually checks the output printed to the console. All of my changes have been at the point of display, so I don't know how you'd like to test that. The closest the old tests get is to test diffs_to_print with various inputs, but that's gone now, and wouldn't have caught any of the issues I've identified anyway. Please note that, quite inconveniently, it seems to not be possible to pass a TextDiff struct around.

I'm sorry this little PR has ballooned, but I'm quite happy you haven't done a release yet. I'm hoping adopting similar will end up being a nice simplification, but there are just a few things still to resolve.