Skip to content

#119 #120 Fix make_patch() to avoid casting numeric string to int on item moved #122

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

Merged
merged 3 commits into from
Mar 2, 2021

Conversation

RyanSept
Copy link
Contributor

@RyanSept RyanSept commented Feb 1, 2021

Addresses #119 #120

@coveralls
Copy link

coveralls commented Feb 1, 2021

Coverage Status

Coverage decreased (-0.06%) to 65.846% when pulling f6b26b2 on RyanSept:119-120-fix-diffbuilder into a9a83b5 on stefankoegl:master.

@stefankoegl
Copy link
Owner

stefankoegl commented Feb 4, 2021

There seems to be an issue with a pypy build, but that seems unrelated to your pull request. I will have a closer look in the next days.

@stefankoegl stefankoegl self-assigned this Feb 4, 2021
@RyanSept
Copy link
Contributor Author

RyanSept commented Feb 4, 2021

Thanks 🙌

jsonpatch.py Outdated
Comment on lines 803 to 806
if type(op.key) == int:
added_item = op.pointer.to_last(self.dst_doc)[0]
if type(added_item) == list:
Copy link
Owner

Choose a reason for hiding this comment

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

Could you please just add a comment here to clarify for posterity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've done so now. Please let me know if there's anything else I should add in the comment. It was a bit difficult to express what the _on_undo_add call accomplishes and I think it'd be valuable to mention that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stefankoegl kind reminder on this

@stefankoegl stefankoegl merged commit 25762af into stefankoegl:master Mar 2, 2021
@stefankoegl
Copy link
Owner

Thanks :)

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.

3 participants