search

fonsp / Pluto.jl

🎈 Simple reactive notebooks for Julia
https://plutojl.org/
MIT License
5k stars 296 forks source link

Improve GitHub diffs by wrapping lines #1643

Closed juliohm closed 3 years ago

juliohm commented 3 years ago

Pluto is amazing in the sense that we can work as a team in interactive material with students and still be able to review the text on GitHub without opening the actual notebook. However, when we have markdown cells with long paragraphs, the diffs are very long on GitHub and we can't visualize them easily to suggest changes.

Would it be easy to always wrap cell content into a maximum number of columns automatically for the end user? This could be a feature limited to markdown or a feature that applies to all cells, including code as well. It doesn't need to change the view of the code in Pluto, the wrapping of lines would only happen in the background.

fonsp commented 3 years ago

Thanks for the feedback! Github already diffs within lines, see e.g. https://github.com/fonsp/Pluto.jl/commit/8f5ebc85cc7405b21a9ca56507c467cf46ba802d#diff-ca030c0acd905c554e012f62ff0b99439750e464de5285068a8f4bef978f1d49 Maybe you can send an example of a diff that is hard to visualize?

Schermafbeelding 2021-11-08 om 13 03 59

I do see how wrapping lines can make some diffs easier to see, but others will get more complicated: If a student makes a change at the start of a long series of wrapped lines, then it might cause all wrapped lines to change, because the wrap is shifted by a word.

juliohm commented 3 years ago

Sorry I think I wasn't very clear. What I meant was that very long lines don't get wrapped with an actual EOL character. For example, there is this very long paragraph that we need to scroll to the right to review:

image

The screenshot contains only the first sentence of the full paragraph, which is 10 times longer.

If a student makes a change at the start of a long series of wrapped lines, then it might cause all wrapped lines to change, because the wrap is shifted by a word.

That is a very good point, I didn't thought about that. I wonder if there is a smart solution to updating the lines that would minimize the diffs. At the end of the day if Pluto added EOL for us in diffs, trying to respect a maximum number of columns, maybe we could also figure where a new typed word should be added? We will have N lines already split and would have to decide where the new word should be added? Maybe punctuation could help? Maybe EOL after every period "."?

fonsp commented 3 years ago

I thought about it some more and I don't see a way for Pluto to insert/remove the newline character in the background, without removing intentional newline from existing notebooks. Is it okay if we close this? It seems like a lot of engineering for very little gain

fonsp commented 3 years ago

You might also want to contact github directly if you have some good examples of diffs that can be displayed better. They diff within lines, and they wrap long lines if you enable split view, maybe that is what you are looing for?

juliohm commented 3 years ago

Feel free to close the issue :) I thought it would be a handy feature, but no big deal. It is still possible to scroll with the mouse in these long paragraphs to propose changes in the GitHub diff.

Agree that this sounds like a nice GitHub feature too.