Skip to content
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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Nov 7, 2022
Merged

Support non-string input in show_source #430

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e31bd8d
Support non-string input in show_source
k0kubun Nov 4, 2022
e8b1c9d
Test show_source as a method
k0kubun Nov 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions lib/irb/cmd/show_source.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,24 @@ module IRB

module ExtendCommand
class ShowSource < Nop
class << self
def transform_args(args)
# Return a string literal as is for backward compatibility
if args.empty? || string_literal?(args)
args
else # Otherwise, consider the input as a String for convenience
args.strip.dump
end
end

private

def string_literal?(args)
sexp = Ripper.sexp(args)
sexp && sexp.size == 2 && sexp.last&.first&.first == :string_literal
end
end

def execute(str = nil)
unless str.is_a?(String)
puts "Error: Expected a string but got #{str.inspect}"
Expand Down
9 changes: 8 additions & 1 deletion lib/irb/context.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -484,9 +484,16 @@ def evaluate(line, line_no, exception: nil) # :nodoc:
end

# Transform a non-identifier alias (ex: @, $)
command = line.split(/\s/, 2).first
command, args = line.split(/\s/, 2)
if original = symbol_alias(command)
line = line.gsub(/\A#{Regexp.escape(command)}/, original.to_s)
command = original
end

# Hook command-specific transformation
command_class = ExtendCommandBundle.load_command(command)
if command_class&.respond_to?(:transform_args)
line = "#{command} #{command_class.transform_args(args)}"
end

set_last_value(@workspace.evaluate(self, line, irb_path, line_no))
Expand Down
14 changes: 14 additions & 0 deletions lib/irb/extend-command.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,20 @@ def irb_context

]

# Convert a command name to its implementation class if such command exists
def self.load_command(command)
command = command.to_sym
@EXTEND_COMMANDS.each do |cmd_name, cmd_class, load_file, *aliases|
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
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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 with require_relative "cmd/show_source".
  • So by the time the irb_show_source method is reached, the definition should always be there. And therefore the require_relative I linked is obsolete now.

Copy link
Member Author

@k0kubun k0kubun Nov 7, 2022 •
edited
Loading

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.

Copy link
Member

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.

Copy link
Member

@st0012 st0012 Nov 7, 2022 •
edited
Loading

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")?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 done e8b1c9d

end
return ExtendCommand.const_get(cmd_class, false)
end
nil
end

# Installs the default irb commands:
#
# +irb_current_working_workspace+:: Context#main
Expand Down
34 changes: 34 additions & 0 deletions test/irb/test_cmd.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -547,6 +547,40 @@ def test_ls_with_no_singleton_class
end

def test_show_source
input = TestInputMethod.new([
"show_source IRB.conf\n",
])
IRB.init_config(nil)
workspace = IRB::WorkSpace.new(self)
IRB.conf[:VERBOSE] = false
irb = IRB::Irb.new(workspace, input)
IRB.conf[:MAIN_CONTEXT] = irb.context
irb.context.return_format = "=> %s\n"
out, err = capture_output do
irb.eval_input
end
assert_empty err
assert_match(%r[/irb\.rb], out)
end

def test_show_source_method
input = TestInputMethod.new([
"p show_source('IRB.conf')\n",
])
IRB.init_config(nil)
workspace = IRB::WorkSpace.new(self)
IRB.conf[:VERBOSE] = false
irb = IRB::Irb.new(workspace, input)
IRB.conf[:MAIN_CONTEXT] = irb.context
irb.context.return_format = "=> %s\n"
out, err = capture_output do
irb.eval_input
end
assert_empty err
assert_match(%r[/irb\.rb], out)
end

def test_show_source_string
input = TestInputMethod.new([
"show_source 'IRB.conf'\n",
])
Expand Down