-
Notifications
You must be signed in to change notification settings - Fork 64
Make @opt_out rrule(...) automatically qualify rrule namespace as ChainRulesCore.rrule
#546
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
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Codecov Report
@@ Coverage Diff @@
## main #546 +/- ##
==========================================
+ Coverage 93.41% 93.45% +0.04%
==========================================
Files 15 15
Lines 865 871 +6
==========================================
+ Hits 808 814 +6
Misses 57 57
Continue to review full report at Codecov.
|
This claim is to strong, there is still all kinds of evil 😂 But more seriously for our own future reference it would be good for this PR to have clearer title and description. As i understand it the core of the problem was if the name |
src/rule_definition_tools.jl
Outdated
| _target_rewrite!(qt::QuoteNode, rule_norule) = _target_rewrite!(qt.value, rule_norule) | ||
| function _target_rewrite!(call_target::Symbol, rule_norule) | ||
| return if call_target == :rrule && rule_norule == :norule | ||
| :(ChainRulesCore.no_rrule) |
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 think
| :(ChainRulesCore.no_rrule) | |
| :($ChainRulesCore.no_rrule) |
might work to actually make this function even if the module name ChainRulesCore isn't in the caller's scope.
If it does then these test could be moved to the isolated macro testing module.
(or a similar setup could be made for this file for testing purposes)
See the setup in
https://github.com/JuliaDiff/ChainRulesCore.jl/blob/main/test/rule_definition_tools.jl#L286
oxinabox
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.
I trust you to make appropriate changes then merge when happy
Co-authored-by: Frames Catherine White <[email protected]>
@opt_out not evil@opt_out rrule(...) automatically qualify rrule namespace as ChainRulesCore.rrule
Closes #545