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

exposing Automerge::Diff through to Swift #183

Merged
merged 11 commits into from
Jun 7, 2024

Conversation

miguelangel-dev
Copy link
Contributor

@miguelangel-dev miguelangel-dev commented Jun 6, 2024

Bindings for being able to compare two points of the document history.

Related documentation: https://docs.rs/automerge/latest/automerge/struct.Automerge.html#method.diff

@heckj heckj self-requested a review June 6, 2024 15:32
Copy link
Collaborator

@heckj heckj left a comment

Choose a reason for hiding this comment

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

Generally this looks great, thank you!

We don't need both difference(from:) and difference(to:) if they're effectively identical APIs with just slightly different parameters. Drop one, and I've made suggestions to change the parameter since to since since the other side that's passed to the core library's difference is the current set of heads.

I'm a unclear on why the WASM build didn't appear to pick up the change. Visually it looks correct to me, but we might need to dig a bit further. We did just update the CI runners to an updated version of Rust, which may have had a follow on effect that's hitting us here.

Sources/Automerge/Document.swift Show resolved Hide resolved
Sources/Automerge/Document.swift Outdated Show resolved Hide resolved
Sources/Automerge/Document.swift Outdated Show resolved Hide resolved
Sources/Automerge/Document.swift Outdated Show resolved Hide resolved
Sources/Automerge/Document.swift Outdated Show resolved Hide resolved
Sources/Automerge/Document.swift Outdated Show resolved Hide resolved
Sources/Automerge/Automerge.docc/Curation/Document.md Outdated Show resolved Hide resolved
@miguelangel-dev
Copy link
Contributor Author

miguelangel-dev commented Jun 6, 2024

... if they're effectively identical APIs with just slightly different parameters. Drop one, and I've made suggestions to change the parameter since to since since the other side that's passed to the core library's difference is the current set of heads.

Probably the API doc is not the best one and this note has confused you:

`from` and `to` do not have to be chronological. Document state can move backward.

Saying both do not need to be chronological (having order) does not mean the result won't change. Here is an example: let's say I append "World" to the existing "Hello"

  • if difference is applied in chronological order, then operations will be "inserts"
  • If difference is applied in reverse order, operations will be "removals"

Any suggestion for the DocC, to make it more clear is more than welcome 😅

I'm a unclear on why the WASM build didn't appear to pick up the change...

Not sure, but it looks like CI is always compiling against the latest XCFramework, instead of building a local one and using it.

Screenshot 2024-06-06 at 19 26 48

@heckj
Copy link
Collaborator

heckj commented Jun 6, 2024

I saw the same thing in the logs re: downloading the XCFramework. It shouldn't be a requirement for builds on Linux or WASI, but I'd agree that this might be the bit that's hurting us. Unfortunately, XCFramework - while downloaded - can't be rebuilt on just Linux, so we don't have the same path to using a "local version".

If you've tried this on your own build servers and it works - flows through WASM builds correctly, etc - then I'm happy to merge this with that check breaking, on the idea that we can pop a release with the Rust updates which theoretically should resolve the issue for future builds after there's an updated point release.

miguelangel-dev and others added 4 commits June 6, 2024 23:01
Co-authored-by: Joseph Heck <heckj@mac.com>
Co-authored-by: Joseph Heck <heckj@mac.com>
Co-authored-by: Joseph Heck <heckj@mac.com>
@miguelangel-dev
Copy link
Contributor Author

miguelangel-dev commented Jun 6, 2024

If you've tried this on your own build servers and it works - flows through WASM builds correctly, etc - then I'm happy to merge this with that check breaking, on the idea that we can pop a release with the Rust updates which theoretically should resolve the issue for future builds after there's an updated point release.

Adding manually missing autogenerate code (headers + automerge.swift), WASM is working 🎉. We should update the Linux bot to generate it automatically without committing it - please, double-check 868e8cd because it contains more changes than I expected. Probably, because of my rust version (nightly-2024-05-23-aarch64-apple-darwin)

side-note: I can revert this latest commit with the autogenerated code.

Copy link
Collaborator

@heckj heckj left a comment

Choose a reason for hiding this comment

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

I got the CI stuff updated - good call - with #185, and merged that in. You can rebase if you want to double check that this PR works without the checked in updates, but don't feel obliged.

Thanks for clarifying that directionality IS reflected based on the two sets of heads provided to difference, makes a lot more sense to have the various API

@miguelangel-dev
Copy link
Contributor Author

I got the CI stuff updated - good call - with #185, and merged that in. You can rebase if you want to double check that this PR works without the checked in updates, but don't feel obliged.

Awesome! I have reverted the commits with the auto-generated code, to keep the PR clean and have rebased changes into this branch. Result: it works like a charm!

Thanks for the parallel work done into #185 - it is now ready to be merged

@heckj heckj merged commit 17238f5 into automerge:main Jun 7, 2024
4 checks passed
@miguelangel-dev miguelangel-dev deleted the migue/automerge-diff branch June 7, 2024 05:51
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.

2 participants