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

ExpressibleBy Protocols #138

Closed
wants to merge 2 commits into from
Closed

ExpressibleBy Protocols #138

wants to merge 2 commits into from

Conversation

lightsprint09
Copy link
Member

Not sure if you like this. By implementing the ExpressibleBy we can get rid of a lot of explicit boilerplate code. Tests get much more easy to read and write. Most application code won't get any better because it relies on dynamic behavior.

. Some prefer to to write the following:
let value: Value = Value.Scalar.String("string")

I would prefer the following:
let value: Value = "string"

Most application code:
let value: Value = .Scalar(.String(someDynamicVariable))

What do you think?

@heckj heckj self-requested a review March 31, 2024 19:05
@heckj
Copy link
Collaborator

heckj commented Mar 31, 2024

It's definitely smoother for adding scalar values within the types that you've chosen to map from the Swift expressibleBy patterns, but it makes it in some ways even more difficult to deal with expressing an explicit UInt (or Counter) in the Automerge document, and has a sort of implicit bias that I'm not sure I'm comfortable with.

I think there's benefit to presenting all the details of the types that Automerge exposes on a sort of equal footing, and because there's not explicit expressible-by mechanisms for the variety of internal Automerge types, I think I'd prefer to avoid adding this kind of thing into place. In my thinking, I'm leaning a lot more into explicit, not implicit, representations of types from the Swift language side of things.

UPDATE: as I thought about it more, I'm not sure if this is a good bias or not. I've poked folks who've recently been using this library in our Discord chat (#swift-automerge) to as for feedback and thoughts, if they want to share, on this PR.

@lightsprint09
Copy link
Member Author

I am fine with that. I will leave this open of now

@alexjg
Copy link
Collaborator

alexjg commented Apr 1, 2024

FWIW we use a very similar pattern in the Rust implementation using Into<ScalarValue> which I think is quite ergonomic. This works because use of From/Into is idiomatic in Rust, so I think it depends on how surprising use of ExpressibltBy* is in the Swift ecosystem

@miguelangel-dev
Copy link
Contributor

miguelangel-dev commented Apr 2, 2024

The use of ExpressibleBy* is quite extended among the Apple community, and intensively used by Apple in their public APIs. Said that, I am not a super fan of them because in some situations we silently produce ambiguity in the type of inference (e.g. between UInt, Int, Double,...).

For that particular case, where the performance is important and it will only work in a subset of the Scalar.Values, IMHO it brings more negative scenarios than benefits in the public API. But I see the potential of introducing it internally for testing purposes, to increase the readability and ergonomic of the tests.

@heckj
Copy link
Collaborator

heckj commented Apr 2, 2024

Based on the feedback here, and in Discord, I'd prefer not to merge this one. It is a convenience, but I'd like to maintain the explicit-ness in the core library API, and leave convenience to wrappers or higher-level things that layer over the core.

@heckj heckj closed this Apr 2, 2024
@heckj heckj deleted the expressibleBy branch May 25, 2024 00:16
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