-
Notifications
You must be signed in to change notification settings - Fork 59
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
raise julia errors #525
base: main
Are you sure you want to change the base?
raise julia errors #525
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #525 +/- ##
==========================================
- Coverage 80.40% 77.92% -2.49%
==========================================
Files 26 26
Lines 1684 1685 +1
==========================================
- Hits 1354 1313 -41
- Misses 330 372 +42 ☔ View full report in Codecov by Sentry. |
src/callback.jl
Outdated
nprotect = 2 | ||
local clos | ||
try | ||
args = protect(sexp_arglist_dots()) | ||
args = RCall.protect(RCall.sexp_arglist_dots()) |
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.
args = RCall.protect(RCall.sexp_arglist_dots()) | |
args = protect(sexp_arglist_dots()) |
src/callback.jl
Outdated
lang = RCall.rlang_p(:function, args, body) | ||
clos = RCall.reval_p(lang) |
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.
lang = RCall.rlang_p(:function, args, body) | |
clos = RCall.reval_p(lang) | |
lang = rlang_p(:function, args, body) | |
clos = reval_p(lang) |
src/callback.jl
Outdated
finally | ||
unprotect(nprotect) | ||
RCall.unprotect(nprotect) |
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.
RCall.unprotect(nprotect) | |
unprotect(nprotect) |
src/callback.jl
Outdated
args = protect(sexp_arglist_dots()) | ||
args = RCall.protect(RCall.sexp_arglist_dots()) |
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 don't need to qualify things from RCall when executing within RCall. (If you were overwriting these methods from an external definition, then you do need that qualification.)
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.
thank you for all the changes. yes indeed I was previously overwriting it but as this got discouraged like an error in julia 1.10 I switched to forking RCall. These were apparently some leftovers
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'm generally onboard with this, but it needs tests of the new behavior. 😄
@palday I took away the |
I believe the test to have is something that checks for the new julia error being raised as expected in this PR, so we don't regress. |
@schlichtanders there is an entry with the comment "callbacks" in the tests folder: https://github.com/JuliaInterop/RCall.jl/blob/master/test/basic.jl#L67 That seems like a good place to put it. 😄 |
fixes #508