-
Notifications
You must be signed in to change notification settings - Fork 605
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
Implement Binding#local_variables and Binding#receiver #3352
Conversation
@@ -54,4 +54,12 @@ def eval(expr, filename=nil, lineno=nil) | |||
|
|||
Kernel.eval(expr, self, filename, lineno) | |||
end | |||
|
|||
def local_variables | |||
eval('local_variables') |
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.
Euh, we shouldn't need (nor want) the use of eval
here. If I'm not mistaken the code currently used for Kernel.local_variables
should be usable in this context, either in the form of a copy-paste (meh) or by using a helper method (one that takes a Rubinius::VariableScope
).
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.
@yorickpeterse Indeed, the use of eval is a bit gross. The reason behind it was only to avoid duplicating Kernel.local_variables
. Would pushing this logic into Rubinius::VariableScope
be an acceptable option?
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.
Yes, from the top of my head I think this can be reduced down to the following:
def local_variables
scope = Rubinius::VariableScope.of_sender
scope.local_variables
end
This can then be used in both Kernel#local_variables
and Binding#local_variables
.
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.
Binding
already has its VariableScope
- it's in the @variables
ivar, from Binding.setup
.
Implement Binding#local_variables and Binding#receiver
end | ||
|
||
def receiver | ||
@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.
Where does @self
originate from? Is this not supposed to just return self
(minus the @
)?
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.
It should indeed be @self
here - self
would return the Binding
instance itself.
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.
Ah, I now see this is set by Rubinius::VariableScope#self=
, which wasn't entirely obvious to find.
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 make it more obvious for others in the future, it seems like the @self
ivar could be renamed to @receiver
, so that Binding#receiver
would just be an attr_reader
.
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.
@jemc I agree. The attr_accessor :self
could be replaced with regular
def self=(recv) ...
and def self ...
methods refering to @receiver
, which would make the purpose of the ivar clearer, without breaking client code depending on self
.
@jemc I'm reverting this, see my notes. |
Continued at #3354 |
Hi everyone, this PR implements Binding#local_variables and Binding#receiver.