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.

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

Stop echoing or storing command calls' nil return value #972

Merged
merged 1 commit into from
Jun 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
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
Loading