Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NBDime REST API changes for git diffs #480

Closed
jaipreet-s opened this issue Jun 11, 2019 · 4 comments
Closed

NBDime REST API changes for git diffs #480

jaipreet-s opened this issue Jun 11, 2019 · 4 comments

Comments

@jaipreet-s
Copy link
Contributor

jaipreet-s commented Jun 11, 2019

Hi @vidartf and others,

I'm touching base on the work proposed to add visual diff in the Git extension in jupyterlab/jupyterlab-git#331

Current world

The nbdime REST API (/nbdime/api/diff) provides diff for at a file level

  1. Two versions of a notebook file (i.e. specifying base and remote)
  2. passing in base as git:$FILE_NAME and that computes the diff between version of the file in the Git working directory and HEAD

Proposed changes

To support diffs across the Git workflow, the proposal was to enhance this API to allow diff'ing between multiple points such as between two arbitrary git refs(equivalant of git diff $OLD_COMMIT $NEW_COMMIT or Diffing the changes in the staging area (equivalent of git diff --staged)

There are two ways to go about this

  1. Add fields to the existing diff API that allow this functionality

    • Pros: Few changes needed to nbdime
    • Cons: Since the current nbdime API works on a single file, the Git extension has to find all the files that changed between the two git refs and invoke the nbdime API multiple times
  2. Add a specific APIs for Git diffs that provide diff for all files based on the Git context passed

    • Pros: 1 API call from the client perspective and no need to pass the file name
    • Cons: Relatively more changes needed to nbdime and a bit specific to Git

Both will work but I wanted to gather any feedback before embarking on this work.

@vidartf
Copy link
Collaborator

vidartf commented Jun 11, 2019

Note: For alternative 1. there is some work already in a branch on my repo (master...vidartf:jlgit). Although it would probably be better to not use the prefix method, if it can be avoided.

For alternative 2, I'd appreciate thoughts on pros/cons of putting this in nbdime, vs implementing this somewhere else (e.g. jupyterlab-git).

@jaipreet-s
Copy link
Contributor Author

jaipreet-s commented Jun 12, 2019

Note: For alternative 1. there is some work already in a branch on my repo (master...vidartf:jlgit). Although it would probably be better to not use the prefix method, if it can be avoided.

I agree - If we go along this route, we'll update the API to have explicit fields for git refs

For alternative 2, I'd appreciate thoughts on pros/cons of putting this in nbdime, vs implementing this somewhere else (e.g. jupyterlab-git).

Good point. One consideration on option 2 is where it is implemented.

Pro of implementing this in the Git extension:

  • The Git logic is encapsulated in the Git extension
  • We'd likely move faster from the Git extension's perspective and have to worry less about breaking nbdime users

Con of implementing this in the Git extension:

I don't think either option is bad, I'm fine to start with one and contribute whatever makes sense back to nbdime.

@vidartf
Copy link
Collaborator

vidartf commented Jun 13, 2019

My current thoughts (open to good arguments though):

  • I'd be very happy to have a good API for requesting diffs of one file between two git refs.
  • Having nbdime manage directory diffing with git sounds like something it is probably best to put somewhere else, as most tools would likely be expected to handle other files than just notebooks any way.

Note: It sounds a little counter-intuitive to me to have a notebook-only directory diffing solution, but there might be something I'm missing.

@vidartf
Copy link
Collaborator

vidartf commented Aug 2, 2019

Fixed by #482.

@vidartf vidartf closed this as completed Aug 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants