-
Notifications
You must be signed in to change notification settings - Fork 53
Raise tracking support for chronos procedures #298
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
Conversation
| raises: [Defect, CatchableError].} = | ||
| # we have to rely on inference here | ||
| {.pop.} | ||
| proc read*(future: Future | RaiseTrackingFuture ): auto = |
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.
just make a separate read function for RTF? the auto return type here is unnecessarily wide as well, there's too many compromises for too little benefit
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.
Indeed, we can avoid it here by making a specialized read for RTF
| return retFuture | ||
|
|
||
| proc cancelAndWait*[T](fut: Future[T]): Future[void] = | ||
| proc cancelAndWait*[T](fut: Future[T]): RaiseTrackingFuture[void, (CancelledError,)] = |
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.
| proc cancelAndWait*[T](fut: Future[T]): RaiseTrackingFuture[void, (CancelledError,)] = | |
| proc cancelAndWait*[T](fut: Future[T]): Future[void] {.asyncraises:[(CancelledError,)] = |
the rewrite to RTF should be macro-handled, no?
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.
We don't have access to macros here (asyncmacro2 requires asyncfutures2)
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 we should move this one elsewhere then - ie cancelAndWait is a composite "utility" operation much like allFutures etc - then we keep "primitives" in asyncfuture2 and maybe one fine day we can get rid of include altogether
|
|
||
| return retFuture | ||
|
|
||
| proc wait*[T, E](fut: RaiseTrackingFuture[T, E], timeout = InfiniteDuration): auto = |
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.
what's the return type here? in public functions, we ideally want to avoid auto both for documentation reasons and because it serves as a type inference guide for the compiler.
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.
It's RTF[T, E + CanceledError] (which I just realized is wrong, should be AsyncTimeoutError)
I don't know a better solution than auto for this kind of procedures (but I don't think it has an impact on type inference)
|
superseded by #454 |
Follow up on #251