-
Notifications
You must be signed in to change notification settings - Fork 448
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
WIP: Pass Trace transparently through withError #3358
base: arrow-2
Are you sure you want to change the base?
Conversation
This is achieved by sealing Raise to check DefaultRaise.isTraced.
Kover Report
|
Turning The implementors of Raise should be very few and far in between, and both provided APIs (
|
Requesting reviews just to get some thoughts on this. I'm not fully convinced that this PR is the right approach, but It's hard to see how else to conditionally trace based on whether the caller is in a traced context or not. Food for thought: with this change and #3349, maybe it's time to redesign |
@@ -143,7 +143,7 @@ public annotation class RaiseDSL | |||
* <!--- KNIT example-raise-dsl-04.kt --> | |||
* <!--- TEST lines.isEmpty() --> | |||
*/ | |||
public interface Raise<in Error> { | |||
public sealed interface Raise<in Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, for me this is a no-go, we really want to keep this one open for extension (we even talk about that in the docs).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that I'm keeping it open by instead providing RaiseWrapper
and TransformingRaise
. Those 2 are the only use cases that I can observe. Most people will use RaiseWrapper
if they're just defining an intersection type (and we can deprecate it when contexts are stable). TransformingRaise
is another pattern where one wants to change the error type right then and there (used in RaiseAccumulate
and SingletonRaise
), and it's slightly more performance than withError
.
Instead of this change, would you prefer we force implementors to provide a val underlyingRaise: Raise<*>
or something along those lines? I didn't want to go through that approach because one can do val underlyingRaise get() = this
which I heavily dislike
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @serras. I understand it's still open for extension, but not in a straightforward way.
Did not yet have the time to properly dive into this, but perhaps we can find a different solution. Perhaps going the other-way around might be better. Something like CoroutineStackFrame
which you can optionally implement, if you need behavior like this one.
We can dynamically check if a Raise
interface implements RaiseFrame
, or similar and then we'd get access to additional properties. That would probably even be possible to add in 1.x
No idea if any of this would work, just spitting some ideas
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've head of teams using CoroutineContext
to pass this information, but unfortunately we don't have suspend
in our raise
function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about introducing some form of interface RaiseWrapper<Error>: Raise<Error> { val underlyingRaise: Raise<*> }
and encouraging users to gradually use it? We can support this feature then for those implementors. Alternatively, we can add an val underlyingRaise
into Raise
directly, with a default impl of returning this
, although this would be a 2.0 feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm also getting distracted by thinking too far into the future. With contexts, we don't want "delegating" implementors of Raise
, because they can always be replaced by composing Raise
and the class in question. Instead, we want "transforming" implementors that actually do something interesting with the error value. Maybe that, as a plan, is unnecessarily ambitious. I'll experiment here with simplifying this, focusing on requiring as little migration as possible.
This is achieved by sealing Raise to check DefaultRaise.isTraced.