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

Use millisecond semantic to match automerge-core #141

Merged
merged 4 commits into from
Apr 2, 2024

Conversation

lightsprint09
Copy link
Member

@lightsprint09 lightsprint09 commented Apr 1, 2024

This changes timestamp to use milliseconds as documented here and discussed in #139 . Date is now used to represent a timestamp, to prevent mistakes when encoding/decoding. Converting Date to FFI representation is now in a single file

@ept
Copy link
Member

ept commented Apr 1, 2024

Hold on a minute, is it actually correct that automerge-core uses milliseconds resolution? I thought it was using seconds.

@ept
Copy link
Member

ept commented Apr 1, 2024

Oh sorry, we're talking about timestamps that appear in the document data. Yes, for those we use milliseconds. I thought we were talking about the change timestamp, which I believe uses integer seconds.

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.

Thanks Lucas

Is there any place that we have a unified document with all the various data representations that we could use to verify the assertion that it's in milliseconds with Javascript and/or Rust? I'd really like to get something that's concrete to verify the assertions of representation like this, especially since it was a mistake I made early on and jsut never caught, since I wasn't using this cross-platform - although that's the goal here.

Sources/Automerge/ScalarValue.swift Outdated Show resolved Hide resolved
@heckj
Copy link
Collaborator

heckj commented Apr 1, 2024

Note for release: we'll want to make sure to broadcast loudly that this changed - if anyone already has Date representations in their Automerge docs created, this will cause all of their Date values to be "broken", since we were representing them differently.

@alexjg
Copy link
Collaborator

alexjg commented Apr 1, 2024

I also missed this so I think having some kind of concrete interop test would be a good idea, though I'm not sure exactly what that would look like.

@alexjg
Copy link
Collaborator

alexjg commented Apr 1, 2024

In terms of written documentation the value section of the binary format docs exists but is quite dense.

@heckj
Copy link
Collaborator

heckj commented Apr 1, 2024

@alexjg I was thinking exactly the same thing just a few minutes earlier. Especially for the various language implementations, I think it would be a great idea. Maybe even just a document that has one of each of the .scalarValue data blobs within it, with specific values, and an external representation. In practice, and looking through the .scalarValue data types, I think the only one that's subject to "local interpretation" is the timestamp value, but as we advance into more "attributed strings" territory, I think the concept of a common reference document will hold us in good stead, and we can make sure that some level of "marks" or attributes/blocks are equally matched in cross-platform representations.

UPDATE: opened a PR to add an interop exemplar document into Automerge core: automerge/automerge#895

lightsprint09 and others added 3 commits April 1, 2024 20:28
Co-authored-by: Joseph Heck <heckj@mac.com>
…nto miliseconds-timestamp

# Conflicts:
#	Sources/Automerge/ScalarValueRepresentable.swift
#	Tests/AutomergeTests/TestScalarValueConversions.swift
@heckj
Copy link
Collaborator

heckj commented Apr 1, 2024

@lightsprint09 I think your rebasing went a bit awry here, GH is complaining about it being "unmergable"

@lightsprint09
Copy link
Member Author

Looks good to me?

@heckj
Copy link
Collaborator

heckj commented Apr 2, 2024

Screenshot 2024-04-02 at 11 47 44 AM

@lightsprint09
Copy link
Member Author

Bildschirmfoto 2024-04-02 um 20 53 13

hmm ;)

@heckj
Copy link
Collaborator

heckj commented Apr 2, 2024

Odd - if you can merge it, go ahead - it's been complaining since you merge back the main branch, but if it's clear for you, awesome.

@lightsprint09 lightsprint09 merged commit d751b07 into main Apr 2, 2024
4 checks passed
@heckj heckj deleted the miliseconds-timestamp branch April 2, 2024 19:57
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.

4 participants