Minor error in diff highlighting of a markdown cell

[juhoautio]

I wonder why the first bullet point is also highlighted although it didn't change.

It can be seen live here: https://app.reviewnb.com/juhoautio/NotebookTest/pull/1/files/

[amit1rrr]

@juhoautio In this case a newline (\n) character got added to the first bullet that's why it's highlighted in green.

As you can see in the text diff here - https://github.com/juhoautio/NotebookTest/pull/1/files#diff-933ae29e83f5704b1f97d0c3ca04fb25R9

I am closing this but feel free to reopen the issue if you have any follow up questions.

[juhoautio]

The github diff is able to show a similar (non-notebook) markdown change "properly" in the rich diff:

https://github.com/juhoautio/NotebookTest/commit/0f8c6dab0404b3d8d56d1b1ed9481a376a69718a?short_path=91b5c4a#diff-91b5c4a22fc6103c73bb91e4a40568f8

..even though it's not highlighting the newline-only change in any visible way in the raw diff:

https://github.com/juhoautio/NotebookTest/commit/0f8c6dab0404b3d8d56d1b1ed9481a376a69718a#diff-91b5c4a22fc6103c73bb91e4a40568f8

I'd expect ReviewNB to display markdown cell diffs in the same way, but of course it's debatable which one is even correct. Maybe you consider this as a possible improvement for ReviewNB, maybe not? :)

---

In some cases github does something like this even in the raw diff, eg. here the changed number of execution_count:

But when the change is newline, I guess there's no similar thing (yet?).

---

To create the initial test file without an ending newline I used this:

printf "%s" "- something" > file.md

[amit1rrr]

@juhoautio I like your suggestion. I like to call our diff as visual diff and since nothing is visually different in your example there is no need to highlight the newline change.

I have gone ahead and deployed the fix. Now we don't highlight newline changes if they are non-visual (like in this case). Refer to screenshot below.

P.S. - There could be some whitespace changes that are harder to detect whether they make visual difference or not (e.g. HTML outputs) so we aren't handling those yet but this is a good start.

Thanks for following up.