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

swift: allow specifying text encoding at document creation #206

Merged
merged 14 commits into from
Jan 24, 2025

Conversation

miguelangel-dev
Copy link
Contributor

@miguelangel-dev miguelangel-dev commented Jan 20, 2025

Related pull-requests:

What is the goal?

Exposes new Automerge TextEncoding flag for creation of documents. It enables us to define the text representation to use in each of the operations without being forced to use Unicode Scalars in Swift.

blockers: pending to merge related PRs, and bump automerge core.

@miguelangel-dev miguelangel-dev changed the title Allow specifying text encoding at document creation swift: allow specifying text encoding at document creation Jan 20, 2025
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've requested a few minor nits/changes in documentation wording for clarity, all as suggested changes. Additionally, I'd like to make sure that the exposed TextEncoding enum is documented through to the external Swift API that it's clear how those values related to Swift encodings.

The tests are the real gem of this PR - thank you - illustrating nicely how the text encoding changes effect cursor positions, etc.

What, exactly, is this holding on before we can/should merge this? Is there an upstream PR that's pending, or is that yet to be established?

Sources/Automerge/Document.swift Outdated Show resolved Hide resolved
Sources/Automerge/Document.swift Outdated Show resolved Hide resolved
rust/src/text_encoding.rs Outdated Show resolved Hide resolved
rust/src/text_encoding.rs Outdated Show resolved Hide resolved
rust/src/text_encoding.rs Outdated Show resolved Hide resolved
@miguelangel-dev
Copy link
Contributor Author

miguelangel-dev commented Jan 21, 2025

I've requested a few minor nits/changes in documentation wording for clarity, all as suggested changes. Additionally, I'd like to make sure that the exposed TextEncoding enum is documented through to the external Swift API so that it's clear how those values relate to Swift encodings.

Good call. Added Swift type and API documentation about TextEncoding in 71d4408

What, exactly, is this holding on before we can/should merge this? Is there an upstream PR that's pending, or is that yet to be established?

Exactly. This PR exposes changes done in upstream PR related to TextEncoding, and Cursor fixes. @alexjg is working on merging and releasing a new automerge version.

@miguelangel-dev miguelangel-dev requested a review from heckj January 21, 2025 18:11
@miguelangel-dev miguelangel-dev requested a review from heckj January 23, 2025 09:25
@miguelangel-dev
Copy link
Contributor Author

miguelangel-dev commented Jan 23, 2025

@heckj requested your review because I changed the ci. Wasm job uses locally compiled libuniffi_automerge, but in mac, we were using the latest release precompiled Automerge. That change will align both and it opens to have two versions of the Automerge library: develop | release - in another way, in develop bindings won't be able to run ci using the latest Automerge library.

side note: PR is ready to merge.

miguelangel-dev added a commit to GoodNotes/automerge-swift that referenced this pull request Jan 24, 2025
@heckj heckj merged commit 917b6b6 into automerge:main Jan 24, 2025
5 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.

2 participants