👋 [GitHub] Improved diff support for notebooks in PRs.

[dynamicwebpaige]

GitHub is exploring options to improve the rendering experience for .ipynb notebooks in PRs on GitHub.com. Today, only raw JSON is shown in the changed file; comments are limited to only the first few lines of JSON that delineate the cell that has been changed; etc. There are certainly many opportunities for improvement, and we are just getting started. 🙂

Rather than building an internal closed-source solution, the GitHub engineering team is hoping to use, scale, and contribute back to community standard libraries. Would it be possible to schedule some time with the nbdime team to discuss whether it could be feasible to prefer this library for the notebook diffing experience in PRs?

Thank you for creating this project, and for your hard work to maintain it! It is appreciated.

[vidartf]

Hi. Throughout the last years I've had calls with different people from the GH team on this topic. I'm always willing to have another one, but please review the previous issues on this as well:
#453
#243

One thing that would probably be needed for nbdime to be useful for GH is #468.

Hope that helps as context!

[gwincr11]

@dynamicwebpaige thank you for the introduction. @vidartf Nice to meet you, thank you for sharing your prior history with this and GitHub. Perhaps it is long past due for a restart.

Our team has been working on standing up our new Notebook service. I have started playing with getting NbDime up and running on the server. It seems that much of our work standing up NbConvert can be re-used and we really just need to setup the NbDime library along side it. I have started spiking this out to learn about the library and did stumble upon the pr you linked, which I agree would be a step in the right direction.

I would love to setup some time to talk and hopefully eventually setup some time to for you and other important contributors to this project to talk with the larger team. I really appreciate your assistance in this matter. ❤️

[gwincr11]

👋 I have been playing around with the package some, it would be lovely to make it easier to get at the javascript in the webapp folder easier since it is so central to how diff displaying works and we want to stay as close to vanilla nbdime as possible.

#468 would be slightly helpful to get merged, the big thing for us would be changing the initializeDiff function in the javascript. Moving this out of diff.ts altogether would help make the js more consumable. I am also not sure the best way to get at the js modules, we will need to figure that out as well, I was thinking it maybe nice to be able to access this as an npm package as well as a python project, or some other similar approach.

I would also like to be able to enable the different ui options separately. I am not sure how the community would like to approach any of these approached changes, but we are happy to help.

[gwincr11]

Oh it looks like there is already an npm package! https://www.npmjs.com/package/nbdime although some documentation would be helpful 😄

[vidartf]

I would love to setup some time to talk and hopefully eventually setup some time to for you and other important contributors to this project to talk with the larger team. I really appreciate your assistance in this matter. ❤️

Yes, let us do this. I'm off for the rest of the year, but let's get in touch in the new year to schedule something!

[gwincr11]

@vidartf wanted to reach back out since it is after the New Year.

Would it be alright if I setup a time to chat with you about setting up NbDime? I can schedule a call using your email you supplied to GitHub, is that ok?

[gwincr11]

I am actually not going to need that pr after all I think I want to tie into things much more as a library and likely use many of the js modules directly.

One thing I am curious to discuss is if there is a way to map back cells to the json they came from, as in the line number in the json of the head or base diff, also how I can alter some of these underlying widgets easily and add functionality to them?

[vidartf]

@gwincr11 Let's set up a call. You should be able to find my email in the commit log :)

[gwincr11]

Thanks @vidartf I will reach out to you. Covid related school closures have made life a bit interesting so I may need to move this out a little bit.

[alecglen]

After tracing through history I'm glad to see some relatively recent movement on this great potential feature! Just curious about the current state if you could please provide an update?

[vidartf]

Nobody has reached out so far :)

[raybellwaves]

Curious if anyone has created a GitHub action which using nbdime to help review PRs.

[raybellwaves]

I started tinkering with https://github.com/marketplace/actions/nbdime-git-diff feedback and/or PRs are welcome

[gwincr11]

@vidartf sorry for the delay and radio silence. I have reached out via the email I was able to find on you GitHub profile. Cheers!

[vidartf]

This is now complete! Thanks again @gwincr11