search

automerge / automerge-swift

Swift language bindings presenting Automerge
https://automerge.org/automerge-swift/documentation/automerge/
MIT License
217 stars 11 forks source link

ExpressibleBy Protocols #138

Closed lightsprint09 closed 3 months ago

lightsprint09 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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

alexjg commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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.