Don't echo an expression's result when it ends with a semicolon #669
Conversation
| class EchoingTest < IntegrationTestCase | ||
| def test_irb_echos_by_default | ||
| write_ruby <<~'RUBY' | ||
| binding.irb |
There was a problem hiding this comment.
For some reason, opening IRB with IRB.start never works in integration tests.
# this hangs and prints nothing
write_ruby <<~'RUBY'
require "irb"
IRB.start(__FILE__)
RUBYSimilarly, introducing a helper like run_irb to make it run exe/irb doesn't work either.
So I decided to go with binding.irb for now and investigate this further later.
| @@ -562,7 +562,8 @@ def eval_input | |||
| is_assignment = assignment_expression?(line) | |||
| evaluate_line(line, line_no) | |||
|
|
|||
| if @context.echo? | |||
| # Don't echo if the line ends with a semicolon | |||
| if @context.echo? && !line.match?(/;\s*\z/) | |||
There was a problem hiding this comment.
I think this simple implementation is good enough.
However, I found a false positive. Usingtokens.last will be more strict. What do you think?
puts %;string;puts +% ; puts <<';'
;puts <<' '
heredoc;
And maybe a false negative, maybe not. (Checking this will be very hard)
puts(<<A);
AThere was a problem hiding this comment.
Great catch! And yeah I agree it's better to make this check with Ripper. But I'd like to avoid invoking it outside of RubyLex and want to make this property an yield argument of each_top_level_statement instead (like #670). WDYT?
There was a problem hiding this comment.
I think It should be in RubyLex too if it uses ripper.
I'll merge this pull request first and let's use ripper in the followup pull request maybe after #670 👍
|
Thanks for supporting this! Very useful for example when dealing an For what it's worth, Pry's implementation seems limited to ending with a semicolon: a space after the semicolon results in the value being output. I like that this is a bit smarter. Hopefully the false positives can be fixed in the future, though they should be pretty rare. 🤞 |
We already skipped history integration tests in core CI in #675 due to suspicion on nested IRB sessions don't work on certain operating systems. But after #669, the evaluation integration test also started to fail on some Core CI suites. So, it looks like the integration test setup may not work in Core CI, at least in some suites. Consider `ruby/irb` already has rather sophisticated test suite, I think it's better to skip the integration tests in core CI for now.
We already skipped history integration tests in core CI in #675 due to suspicion on nested IRB sessions don't work on certain operating systems. But after #669, the evaluation integration test also started to fail on some Core CI suites. So, it looks like the integration test setup may not work in Core CI, at least in some suites. Consider `ruby/irb` already has rather sophisticated test suite, I think it's better to skip the integration tests in core CI for now.
|
Nice. Thanks. Maybe it should be also mentioned in docs somewhere. |
Closes #656