-
Notifications
You must be signed in to change notification settings - Fork 5
Support 0.6 #8
Support 0.6 #8
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8 +/- ##
=========================================
+ Coverage 75.07% 78.4% +3.32%
=========================================
Files 6 6
Lines 333 338 +5
=========================================
+ Hits 250 265 +15
+ Misses 83 73 -10
Continue to review full report at Codecov.
|
src/executors.jl
Outdated
), | ||
on_error_inner! | ||
) | ||
@static if VERSION < v"0.6.0-dev.2042" |
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.
Might be cleaner to do something like
retry_args = @static if VERSION < v"0.6.0-dev.2042"
(retries(exec), Base.DEFAULT_RETRY_MAX_DELAY)
else
ExponentialBackoff(; n=retries(exec))
end
f = wrap_on_error(wrap_retry(run_inner!, allow_retry(retry_on(exec)), retry_args...))
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.
retry_args = @static if VERSION < v"0.6.0-dev.2042"
(allow_retry(retry_on(exec)), retries(exec), Base.DEFAULT_RETRY_MAX_DELAY)
else
(ExponentialBackOff(; n=retries(exec)), allow_retry(retry_on(exec)))
end
f = wrap_on_error(
wrap_retry(
run_inner!,
retry_args...,
),
on_error_inner!
)
How's this?
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.
Yeah, that seems fine. It's annoying that the ordering of the arguments changed as well, but I think this clearly identifies what's actually different.
@@ -129,7 +129,7 @@ function process_nodes!(ex::Expr, ctx_sym::Symbol) | |||
elseif ex.head === :dispatchinclude | |||
process_include!(ex, ctx_sym) | |||
else | |||
map!(x->process_nodes!(x, ctx_sym), ex.args) | |||
map!(x->process_nodes!(x, ctx_sym), ex.args, ex.args) |
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.
These maps are kinda ugly. Do you know why map!(f, collection)
was removed?
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.
In 0.5 asyncmap throws a CompositeException of the errors collected, while in 0.6 it will simply throws the first error it finds. I've made note of this difference in the `dispatch!` docstrings and updated the tests to handle each julia version explicitly.
Adds some very specific version checks and fixes some (not all) deprecations.