Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Protocol for extending the variables pane #560
base: main
Are you sure you want to change the base?
Protocol for extending the variables pane #560
Changes from 18 commits
97ee312
cc1285b
7926192
90ee3ab
501b343
6d4f7e8
ba1c37d
94a4db2
96783ba
2856256
1390024
891e9e4
5ac78f0
92e8c30
d3210b8
e85af96
a2f876c
8676851
8551bf3
d3565fe
c889216
63976ad
9ba6a7e
648b76c
16d4ab8
aadef2b
51111c0
27b7f02
a21dc37
ff887ff
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
To avoid inlining the method in the call, we could assign the function in a child of global (either as generic or generic + class) and evaluate there.
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.
As it's currently implemented
method
is not necessarily a function. It could be a call object, a symbol or a function.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 be in its own module one level above?
Or renamed to
ArkVariableGenerics
.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.
Do you have a preference? It feels like this could be used elsewhere, so we could move to its own module above.
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've moved one level above in ff887ff . Happy to rever if you prefer making it specific for the variables pane.
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.
Since this is rather generic infrastructure, we might want to allow registering methods for base types in the future (only us would be allowed to)?
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.
Any reason not to take a consuming
self
(as opposed to&self
)?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.
My reasoning was that a static method would make more clear that we're actually modfying some global state and not just an object state. But loooks weird indeed - fixed in 16d4ab8
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 can replace this with
RCall
which works similarly toRFunction
(the latter is implemented in terms of the former).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.
But why not pass a function to
register_method()
? To avoid inlining objects in R calls? If so needs a comment, the implicit evaluation is surprising.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.
IIRC, the motivation for storing a call
pkgname:::methodname
instead of the closure object was to ensure that:devtools::load_all()
and similar functions (no risk of calling an outdated method from a stale cache);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 updated to use
RCall
in aadef2bThere 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.
Neat!