-
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
Clean up and add docstrings #539
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #539 +/- ##
==========================================
+ Coverage 80.65% 80.69% +0.03%
==========================================
Files 26 26
Lines 1701 1704 +3
==========================================
+ Hits 1372 1375 +3
Misses 329 329 ☔ View full report in Codecov by Sentry. |
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 cleaning this code up. I have a couple of minor suggestions.
I noticed that the tests on nightly are failing, apparently because of a change in the REPL implementation. I assume the failure is unrelated to these changes. Is that the case?
I'll let @palday handle the merging of the changes in base into this branch. Those kinds of git operations make me want to run away and hide. |
Yes, nightly has been failing on all recent PRs. |
Co-authored-by: Alex Arslan <[email protected]>
Co-authored-by: Alex Arslan <[email protected]>
Co-authored-by: Alex Arslan <[email protected]>
@@ -213,23 +505,26 @@ mutable struct RObject{S<:Sxp} | |||
r | |||
end | |||
# SymSxps are not garbage collected, so preserve not necessary. | |||
RObject{S}(p::Ptr{SymSxp}) where S = new{S}(p) | |||
RObject{SymSxp}(p::Ptr{SymSxp}) = new{SymSxp}(p) |
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.
This isn't functionally the same as what was written but I'm not sure whether what was written before was intentional. It was implicitly relying on convert(Ptr{S}, p::Ptr{SymSxp})
but I don't know enough about how this package works to know whether that's a sensible conversion to do. With this change, RObject{S}(::Ptr{SymSxp})
for S != SymSxp
will be a MethodError
, won't it?
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, I think it will be a method error, but I think that having a method error there makes sense. The usual call would be RObject(p)
, which invokes this definition:
RObject(p::Ptr{S}) where S<:Sxp = RObject{S}(p)
and thus would always use the narrowest possible construction.
No description provided.