-
Notifications
You must be signed in to change notification settings - Fork 64
Deprecate calling thunks like functions #375
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
Always use unthunk instead.
mzgubic
left a comment
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.
Thanks for the PR! Have you checked if there are any references to calling thunks in the docs?
Let me know once you are happy so I can merge
| function (x::InplaceableThunk)() | ||
| Base.depwarn("`(x::InplaceableThunk)()`` is deprecated, use `unthunk(x)`", :call_InplaceableThunk) | ||
| unthunk(x) | ||
| end |
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.
Could we just replace both of these with
function (x::AbstractThunk)()
Base.depwarn("`(x::AbstractThunk)()`` is deprecated, use `unthunk(x)`", :call_AbstractThunk)
return unthunk(x)
end?
Also, BlueStyle.jl uses explicit return
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.
You mean, use AbstractThunk in the depwarn message for both of them?
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.
Good point, should have been something like
function (x::T)() where T <: AbstractThunk
Base.depwarn("`(x::T)()`` is deprecated, use `unthunk(x)`", :call_AbstractThunk)
return unthunk(x)
endThere 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 don't think we can do that - ChainRulesCore is backward compatible with Julia v1.0, and (x::AbstractThunk)() = ... would require >= v1.3, right? And (x::T)() where T <: AbstractThunk = ... even v1.6 won't allow.
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.
Thanks for checking 1.0. With the risk of belabouring the idea:
for T in (:Thunk, :InplaceableThunk)
@eval function (x::$T)()
Base.depwarn("`(x::" * string($T) * ")()` is deprecated, use `unthunk(x)`", Symbol(:call_, $(T)))
return unthunk(x)
end
endshould work. Feel free to ignore if you think that the current way is better (but please do add return)
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.
Sure - just updated the PR, using this variant.
|
|
||
| @testset "Deprecated: calling thunks should call inner function" begin | ||
| @test_deprecated (@thunk(3))() == 3 | ||
| @test_deprecated (@thunk(@thunk(3)))() isa Thunk |
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.
Should we also test InplaceableThunk?
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.
Should we also test InplaceableThunk?
Well, we didn't before, but I can add it.
I hope I found and changed all of them ... |
Always use
unthunkinstead.Closes #374 .