-
Notifications
You must be signed in to change notification settings - Fork 121
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
Support non-string input in show_source #430
Conversation
next if cmd_name != command && aliases.all? { |alias_name, _| alias_name != command } | ||
|
||
if !defined?(ExtendCommand) || !ExtendCommand.const_defined?(cmd_class, false) | ||
require_relative load_file |
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 line can be removed because of this change?
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.
No, this require_relative is executed only when it's used at the beginning of a file, but because it's a method, it could still be called in the middle of a line. So I don't think it's still safe to remove 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.
Sorry I don't get it. Here's my understanding:
- This piece of code is always run before the command methods (e.g.
irb_show_source
) being called. - It checks if the command definition (
ExtendedCommand::ShowSource
) has been loaded yet. If it hasn't, we load it here withrequire_relative "cmd/show_source"
. - So by the time the
irb_show_source
method is reached, the definition should always be there. And therefore therequire_relative
I linked is obsolete now.
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.
So, if you look at my comment, the key is "in the middle of a line". I intend to invoke ExtendCommandBundle.load_command
for commands like irb_show_source ERB#result
, but not for p irb_show_source("ERB#result")
, which is technically still possible and might be still used for debugging IRB itself or something we can't imagine.
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 didn't know that's an expected usage, thanks for sharing. I don't see a point of doing that though because we generally don't care about what the command returns but its side-effects (printing to stdin/out).
But I think supporting it or not is outside of the scope of this 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.
With that being said, do you think you can add another test case for p irb_show_source("ERB#result")
?
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.
👍 done e8b1c9d
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.
🎉
(ruby/irb#430) * Support non-string input in show_source * Test show_source as a method
This PR allows a non-String literal input in
show_source
, just like Pry.Example