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::get_marks through to Swift #186

Merged
merged 13 commits into from
Jun 14, 2024

Conversation

miguelangel-dev
Copy link
Contributor

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

Bindings for being able to retrieve the marks of a given position without traversing the ops of the full document.

https://automerge.org/automerge/automerge/struct.Automerge.html#method.get_marks

@miguelangel-dev miguelangel-dev changed the title Migue/fetch mark index exposing Automerge::get_marks through to Swift Jun 11, 2024
@heckj
Copy link
Collaborator

heckj commented Jun 11, 2024

Thanks @miguelangel-dev, I'll take a look in depth a little later today or tomorrow, WWDC is dominating my attention right now. In the meantime, Marks has some underlying changes that we should get in before dropping this in place - some API updates that align to the latest core library that I have in some commits in the WIP Blocks PR (#171). I'll pull those out of the existing PR and get those set up so we can merge them without waiting for the full detail work on the Blocks API

@heckj
Copy link
Collaborator

heckj commented Jun 11, 2024

@miguelangel-dev I don't think there'll be a serious conflict, but go ahead and rebase over the #171 merge, and things should be good. Biggest detail change here was the internal API switching from Value to ScalarValue inside marks, which I wanted to get in if we were expanding the marks API with these bits.

@heckj heckj self-requested a review June 11, 2024 21:08
@miguelangel-dev miguelangel-dev force-pushed the migue/fetch-mark-index branch from 2788b02 to 1834d62 Compare June 11, 2024 23:18
@miguelangel-dev
Copy link
Contributor Author

miguelangel-dev commented Jun 11, 2024

@miguelangel-dev I don't think there'll be a serious conflict, but go ahead and rebase over the #171 merge, and things should be good. Biggest detail change here was the internal API switching from Value to ScalarValue inside marks, which I wanted to get in if we were expanding the marks API with these bits.

Thanks for moving and merging. I have rebased it against #171 - it should be ready to review 🙏🏻

rust/src/doc.rs Outdated Show resolved Hide resolved
rust/src/mark.rs Show resolved Hide resolved
rust/src/mark.rs 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
@miguelangel-dev
Copy link
Contributor Author

Thanks both for the code review, I have addressed all your suggestions. It is ready for a second round 🙏🏻

@miguelangel-dev miguelangel-dev requested review from heckj and alexjg June 12, 2024 22:01
@miguelangel-dev
Copy link
Contributor Author

Sorry, added a latest minor change to revert an unnecessary public accessibility into the cursor c6798ff

@heckj heckj merged commit 76e59a3 into automerge:main Jun 14, 2024
4 checks passed
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