-
Notifications
You must be signed in to change notification settings - Fork 62
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
Add opting out of rules #398
Conversation
Codecov Report
@@ Coverage Diff @@
## master #398 +/- ##
==========================================
- Coverage 92.32% 92.12% -0.20%
==========================================
Files 14 14
Lines 756 775 +19
==========================================
+ Hits 698 714 +16
- Misses 58 61 +3
Continue to review full report at Codecov.
|
$(esc(no_rule_target)) = nothing | ||
$(esc(expr)) = nothing |
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 this escaping will mean this doesn't work in anything that doesn't have ChainRulesCore imported.
It needs a test in the isolated scope testset.
But I am ok leaving that for a follow up PR.
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.
Sorry, took me a while. Looks good overall, just a few clarifying questions and typos
Co-authored-by: Miha Zgubic <[email protected]>
Co-authored-by: Miha Zgubic <[email protected]>
closes #377
This is actually very breaking so can't be merged til after we tag 1.0-DEV.
It will not break anything in practice til something uses it though.
and it won't break Diffactor.
It will break Zygote and Nabla.
Before mergjng this I want to make the corresponding changes to ChainRulesOverload generation,
and Zygote.