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

Handle addrange/removerange conflicts by splitting. #491

Merged

Conversation

itamarst
Copy link
Contributor

@itamarst itamarst commented Jul 29, 2019

Fixes #488.

Existing code assumes MergeDecision with removerange is only ever paired with patch, so this prevents blowups.

This is based on initial patch from @vidartf.

Existing code assumes MergeDecision with removerange is only ever paired with
patch, so this prevents blowups.
Copy link
Collaborator

@vidartf vidartf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for cleaning this up! Only a minor point to note. Did you test it with your case? Would you be able to encode that as a test in the merge/model.spec.ts ?

packages/nbdime/src/merge/model/notebook.ts Outdated Show resolved Hide resolved
@itamarst
Copy link
Contributor Author

I did test it, and I can make those two improvements (hasEntries and test) later in the week, probably.

@itamarst
Copy link
Contributor Author

itamarst commented Aug 6, 2019

OK, added tests, using hasEntries, and small fix to code based on modeling on the other tests.

@vidartf
Copy link
Collaborator

vidartf commented Aug 7, 2019

Thanks for this, it looks good!

@vidartf vidartf merged commit cdadb44 into jupyter:master Aug 7, 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

Successfully merging this pull request may close these issues.

Conflicting merge decision with both addrange and removerange blows up UI
2 participants