Skip to content

Commit

Permalink
Improve how command calls' return value is handled
Browse files Browse the repository at this point in the history
In #934, we changed command calls to return nil only. This PR improves
the behaviour even further by:

- Not echoing the `nil` returned by command calls
- Not overriding previous return value stored in `_` with the
  `nil` from commands
  • Loading branch information
st0012 committed Jun 16, 2024
1 parent 35de7da commit a4554d4
Show file tree
Hide file tree
Showing 4 changed files with 28 additions and 24 deletions.
1 change: 0 additions & 1 deletion lib/irb/context.rb
Original file line number Diff line number Diff line change
Expand Up @@ -602,7 +602,6 @@ def evaluate(statement, line_no) # :nodoc:
set_last_value(result)
when Statement::Command
statement.command_class.execute(self, statement.arg)
set_last_value(nil)
end

nil
Expand Down
2 changes: 1 addition & 1 deletion lib/irb/statement.rb
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ def is_assignment?
end

def suppresses_echo?
false
true
end

def should_be_handled_by_debugger?
Expand Down
34 changes: 12 additions & 22 deletions test/irb/test_command.rb
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ def test_measure
)

assert_empty err
assert_match(/\A(TIME is added\.\n=> nil\nprocessing time: .+\n=> 3\n=> nil\n=> 3\n){2}/, out)
assert_match(/\A(TIME is added\.\nprocessing time: .+\n=> 3\n=> 3\n){2}/, out)
assert_empty(c.class_variables)
end

Expand All @@ -266,7 +266,7 @@ def test_measure_keeps_previous_value
)

assert_empty err
assert_match(/\ATIME is added\.\n=> nil\nprocessing time: .+\n=> 3\nprocessing time: .+\n=> 3/, out)
assert_match(/\ATIME is added\.\nprocessing time: .+\n=> 3\nprocessing time: .+\n=> 3/, out)
assert_empty(c.class_variables)
end

Expand All @@ -291,7 +291,7 @@ def test_measure_enabled_by_rc
)

assert_empty err
assert_match(/\Aprocessing time: .+\n=> 3\n=> nil\n=> 3\n/, out)
assert_match(/\Aprocessing time: .+\n=> 3\n=> 3\n/, out)
end

def test_measure_enabled_by_rc_with_custom
Expand Down Expand Up @@ -321,7 +321,7 @@ def test_measure_enabled_by_rc_with_custom
conf: conf,
)
assert_empty err
assert_match(/\Acustom processing time: .+\n=> 3\n=> nil\n=> 3\n/, out)
assert_match(/\Acustom processing time: .+\n=> 3\n=> 3\n/, out)
end

def test_measure_with_custom
Expand Down Expand Up @@ -353,7 +353,7 @@ def test_measure_with_custom
)

assert_empty err
assert_match(/\A=> 3\nCUSTOM is added\.\n=> nil\ncustom processing time: .+\n=> 3\n=> nil\n=> 3\n/, out)
assert_match(/\A=> 3\nCUSTOM is added\.\ncustom processing time: .+\n=> 3\n=> 3\n/, out)
end

def test_measure_toggle
Expand Down Expand Up @@ -385,7 +385,7 @@ def test_measure_toggle
)

assert_empty err
assert_match(/\AFOO is added\.\n=> nil\nfoo\n=> 1\nBAR is added\.\n=> nil\nbar\nfoo\n=> 2\n=> nil\nbar\n=> 3\n=> nil\n=> 4\n/, out)
assert_match(/\AFOO is added\.\nfoo\n=> 1\nBAR is added\.\nbar\nfoo\n=> 2\nbar\n=> 3\n=> 4\n/, out)
end

def test_measure_with_proc_warning
Expand All @@ -410,7 +410,7 @@ def test_measure_with_proc_warning
)

assert_match(/to add custom measure/, err)
assert_match(/\A=> 3\n=> nil\n=> 3\n/, out)
assert_match(/\A=> 3\n=> 3\n/, out)
assert_empty(c.class_variables)
end
end
Expand All @@ -429,8 +429,7 @@ def test_irb_source
/=> "bug17564"\n/,
/=> "bug17564"\n/,
/ => "hi"\n/,
/ => nil\n/,
/=> "hi"\n/,
/ => "hi"\n/,
], out)
end

Expand All @@ -457,8 +456,7 @@ def test_irb_load
/=> "bug17564"\n/,
/=> "bug17564"\n/,
/ => "hi"\n/,
/ => nil\n/,
/=> "bug17564"\n/,
/ => "bug17564"\n/,
], out)
end

Expand Down Expand Up @@ -760,10 +758,7 @@ def test_ls_with_no_singleton_class

class ShowDocTest < CommandTestCase
def test_show_doc
out, err = execute_lines(
"show_doc String#gsub\n",
"\n",
)
out, err = execute_lines("show_doc String#gsub")

# the former is what we'd get without document content installed, like on CI
# the latter is what we may get locally
Expand All @@ -776,15 +771,10 @@ def test_show_doc
end

def test_show_doc_without_rdoc
out, err = without_rdoc do
execute_lines(
"show_doc String#gsub\n",
"\n",
)
_, err = without_rdoc do
execute_lines("show_doc String#gsub")
end

# if it fails to require rdoc, it only returns the command object
assert_match(/=> nil\n/, out)
assert_include(err, "Can't display document because `rdoc` is not installed.\n")
ensure
# this is the only way to reset the redefined method without coupling the test with its implementation
Expand Down
15 changes: 15 additions & 0 deletions test/irb/test_irb.rb
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,21 @@ def test_underscore_stores_last_result
assert_include output, "=> 12"
end

def test_commands_dont_override_stored_last_result
write_ruby <<~'RUBY'
binding.irb
RUBY

output = run_ruby_file do
type "1 + 1"
type "ls"
type "_ + 10"
type "exit!"
end

assert_include output, "=> 12"
end

def test_evaluate_with_encoding_error_without_lineno
if RUBY_ENGINE == 'truffleruby'
omit "Remove me after https://github.com/ruby/prism/issues/2129 is addressed and adopted in TruffleRuby"
Expand Down

0 comments on commit a4554d4

Please sign in to comment.