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

Added commitWith() method to specify message/timestamp and exposing changes via changeByHash() #129

Merged
merged 5 commits into from
Mar 15, 2024

Conversation

bgomberg
Copy link
Contributor

We currently (with the older version of the swift bindings) use the timestamp field of the changes in order to determine when a document was last modified for display to the user in our application. I was directed to #104 where this feature has also been discussed. While changes to the underlying AutoCommit type in the core automerge library would likely be better long-term, this PR aims to provide a more targeted solution for this swift library. In the normal usage of this library with an AutomergeEncoder, the underlying transaction is made at the point that save() is called. Therefore, we can make this transaction more explicit and pass through commit options (message / timestamp).

I also needed a way to retrieve these commit options later, so added a new changeByHash(hash:) method which grabs a full Change object from the underlying automerge document for a given hash. This way, we can easily get the timestamp for the latest changes by calling this method on each head of the document.

I added a new test which showcases both of these new APIs.

As an aside, when I ran the build-xcframework.sh script locally, I ended up with a ton of other local changes to the generated code (mostly style changes). I removed those from the PR but would be great to know if there's a way to make this less noisy moving forwards.

@heckj heckj requested review from alexjg and heckj March 13, 2024 14: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.

Conceptually, looks good to me, but I'd like @alexjg to chime in on the Rust code and any implications for exposing the Change contents that I might be missing.

Was a little surprised you replicated/shadowed Change into Automerge-swift rather than using extensions to add pieces to the underlying one, but no qualms with how you did it - it's explicit and clear on what's happening there.

For the optional metadata save component of this, I would like to make either, or other, of those parameters actually optional and handle the missing pieces inside that method appopriately. Ideally, there'd be tests illustrating and working passing in an optional message, an optional timestamp, or both (which seems pathologically silly - but possible) - and what that means (for example - does calling this method with a nil date mean "please save the current timestamp", or is it functionally the same as calling the non-optional-enabled save() method?)

I added some suggested updates to documentation changes on the save method - there's changes that'll be needed in overall curation and other documentation locations, but I can handle integrating those post-merge without issue.

AutomergeUniffi/automerge.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 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
@heckj heckj self-assigned this Mar 13, 2024
@heckj heckj added the enhancement New feature or request label Mar 13, 2024
AutomergeUniffi/automerge.swift Show resolved Hide resolved
rust/src/doc.rs Outdated Show resolved Hide resolved
@bgomberg
Copy link
Contributor Author

Thanks for the feedback @heckj @alexjg.

I separated out commitWith() as that does seem cleaner than overloading save() like I had before. One thing that's a bit funky, but feels like the behavior I would want given the underlying Rust implementation, is that calling save() results in a nil message and 0 timestamp, whereas calling commitWith() results in a nil message and Date().timeIntervalSince1970 timestamp. I do like having the Swift API populate the date for me, and would personally vote for having the Rust code do the same by default (and thus move the save() behavior to match commitWith(), but could understand not wanting to add that dependency. Regardless, I updated the tests to explicitly exercise and demonstrate the differences.

@heckj heckj requested review from alexjg and heckj March 13, 2024 22:17
@bgomberg bgomberg changed the title Added saveWithOptions() method to specify message/timestamp and exposing changes via changeByHash() Added commitWith() method to specify message/timestamp and exposing changes via changeByHash() Mar 13, 2024
@alexjg
Copy link
Collaborator

alexjg commented Mar 14, 2024

Re. the nil message and 0 timestamp. If you call commitWith followed by save then the save shouldn't actually create a new commit. The reason is that save (and all the other methods which trigger a commit) don't do anything unless there are actually outstanding changes, so if you've already saved everything then nothing should happen.

@heckj heckj merged commit e0e67e1 into automerge:main Mar 15, 2024
3 of 4 checks passed
@heckj heckj mentioned this pull request Mar 15, 2024
@bgomberg bgomberg deleted the feat/save-options-and-change branch March 15, 2024 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants