Try redesign using Into instead of explicit conversions.#23
Conversation
With into, there is no need for the 'Compat' conversion because when subtyping is enough then no conversion is needed.
natsukagami
left a comment
There was a problem hiding this comment.
Seems that implementing it this way would break scoping:
test("scoping") {
Result[Int, String]: l1 ?=>
Result[String, Int]: l2 ?=>
val r: Result[Int, Nothing] = Result.Ok(1)
r.ok
}Here the compiler will immediately pick l2 for the Label and tries to apply a conversion (and fail) without retrying with l1.
| @deprecated("No longer needed with redesign using Conversion.into") | ||
| inline given [T]: Conversion[T, T] = identity |
There was a problem hiding this comment.
I think we can just remove this, no need for deprecation
perhaps you could pick a better example because in any case i would expect If i try the following would you expect err to jump to the outer label? given Conversion[Int, Bar] = Bar(_)
class Bar(x: Int)
class Foo
Result[Int, Bar]: l1 ?=>
Result[String, Foo]: l2 ?=>
val r: Result[String, Int] = Result.Err(1)
r.ok
.ok.lengthEdit 2: as a follow up - it doesnt seem possible to infer |
No I don't think so. On second thought I think it's actually good that it doesn't try to jump to the outer label, it could be quite confusing that way.
I think we should be able to do this yeah. |
|
newer design to let you explicitly pass in the label to |
natsukagami
left a comment
There was a problem hiding this comment.
I like this design a lot :D
| "`.ok` cannot be used outside of the `Result.apply` scope." | ||
| ) | ||
| inline label: boundary.Label[Err[E1]] | ||
| )(using inline conv: ConvErr[E, E1]): T = |
There was a problem hiding this comment.
Is having ConvErr a convenient way to have identity conversion without it polluting the user's Conversion search space? It seems pretty neat 🤔
There was a problem hiding this comment.
Yeah I think the goal was mainly to avoid needing the Compat import and also delay the conversion until the error case
| )(using | ||
| @implicitNotFound( | ||
| """`.ok` cannot be used here, as the error types of this Result (${E}) and `Result.apply` (${E1}) are incompatible. Consider changing the error type of `Result.apply`, or provide a conversion between the error types through an instance of the `Conversion` trait: | ||
| opaque type ConvErr[E, E1] = Conversion[Err[E], Err[E1]] |
There was a problem hiding this comment.
| opaque type ConvErr[E, E1] = Conversion[Err[E], Err[E1]] | |
| /** Conversion from error type `E` to error type `E1`. | |
| * An instance is provided for every `E1 <: E`, as well as for every existing implicit [[scala.Conversion Conversion]] instance from `E` to `E1`. */ | |
| opaque type ConvErr[E, E1] = Conversion[Err[E], Err[E1]] |
There was a problem hiding this comment.
Ultimately i backtracked on this design (its still there commented) in favor of eval.ok(using l1)(r) if that control is needed.
|
for now i went back to ok with its hard to decide what to design to keep just because it does add this extra type, but the more complex design is lower runtime cost in all cases the old design is still there as a comment |
With into, there is no need for the 'Compat' conversion because when subtyping is enough then no conversion is needed.