-
Notifications
You must be signed in to change notification settings - Fork 118
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
Fix colorize_code, fix irb crash. #391
Conversation
b242065
to
a7efb93
Compare
|
||
scan(code, allow_last_error: !complete) do |token, str, expr| | ||
if token.nil? |
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.
When I commented out this entire condition, the tests still passed.
I think it's because the str
will still be passed to Reline::Unicode.escape_for_print
at line 145
and then be pushed to colored
at line 150
. So the end result is still the same.
Therefore, this condition doesn't seem to be needed?
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.
It actually works without this change, (I did not notice that) but I think this code is necessary.
I think symbol_state.scan_token(token)
and dispatch_seq
is originally designed to receive Ripper::Lexer::Elem#event
that is not nil.
The current implementation of scan_token and dispatch_seq works just by chance with nil, but I don't think we should pass nil to 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.
I see, thanks for the clear explanation 👍
line_positions << line_positions.last + line.bytesize | ||
end | ||
|
||
on_scan = proc do |elem| |
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.
Do you think it's more understandable if we rewrite the block like this?
on_scan = proc do |elem|
start_pos = line_positions[elem.pos[0] - 1] + elem.pos[1]
# yield uncolorable code
if byte_pos < start_pos
yield(nil, inner_code.byteslice(byte_pos...start_pos), nil)
end
if byte_pos <= start_pos
str = elem.tok
yield(elem.event, str, elem.state)
byte_pos = start_pos + str.bytesize
end
end
Reasons for the changes:
- It's easier to understand if we always put
byte_pos
andstart_pos
at the same position in comparisons. - We can move local variable assignment closer to where it's going to be used.
- In complicated logic like this, I feel traditional if block is easier to understand.
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.
The code you suggested looks more readable. thanks
end | ||
else | ||
lexer.parse.each do |elem| | ||
yield(elem.event, elem.tok, elem.state) | ||
lexer.parse.sort_by(&:pos).each do |elem| |
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.
Tests still pass without this change. Why is it needed?
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 added test case <<A+1\nA
for this.
Another code lexer.parse.reject
in ruby-lex.rb should also be fixed to lexer.parse.sort_by(&:pos).reject
. This change is in another pull request #390
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 removed this sort_by
again and the test still passed 🤔
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.
This else
branch is only executed when ruby < 2.7 and test failed in my environment ruby 2.6.0p0 (2018-12-25 revision 66547) [x86_64-darwin19]
without sort_by
.
[tomoya:irb]% ruby -v
ruby 2.6.0p0 (2018-12-25 revision 66547) [x86_64-darwin19]
[tomoya:irb]% git log -n1
commit da54e7f0813d88546091af5bf220d83a6bd10198 (HEAD -> colorize_every_characters, tompng/colorize_every_characters)
Author: tompng <tomoyapenguin@gmail.com>
Date: Mon Sep 19 14:14:10 2022 +0900
Rewrite on_scan proc to be more readable.
[tomoya:irb]% git diff
[tomoya:irb]% rake test
...skip...
..........................
Finished in 6.179684 seconds.
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------
114 tests, 1158 assertions, 0 failures, 0 errors, 6 pendings, 0 omissions, 0 notifications
94.7368% passed
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------
18.45 tests/s, 187.39 assertions/s
[tomoya:irb]% # delete sort_by(&:pos)
[tomoya:irb]% git diff
diff --git a/lib/irb/color.rb b/lib/irb/color.rb
index 7071696..abe0590 100644
--- a/lib/irb/color.rb
+++ b/lib/irb/color.rb
@@ -196,7 +196,7 @@ module IRB # :nodoc:
on_scan.call(elem)
end
else
- lexer.parse.sort_by(&:pos).each do |elem|
+ lexer.parse.each do |elem|
on_scan.call(elem)
end
end
[tomoya:irb]% rake test
...skip...
..................F
=========================================================================================================================================================================
Failure: test_colorize_code(TestIRB::TestColor)
/Users/tomoya/Documents/github/ruby/irb/test/irb/test_color.rb:269:in `assert_equal_with_term'
/Users/tomoya/Documents/github/ruby/irb/test/irb/test_color.rb:139:in `block in test_colorize_code'
136:
137: tests.each do |code, result|
138: if colorize_code_supported?
=> 139: assert_equal_with_term(result, code, complete: true)
140: assert_equal_with_term(result, code, complete: false)
141:
142: assert_equal_with_term(code, code, complete: true, tty: false)
/Users/tomoya/Documents/github/ruby/irb/test/irb/test_color.rb:137:in `each'
/Users/tomoya/Documents/github/ruby/irb/test/irb/test_color.rb:137:in `test_colorize_code'
Case: colorize_code("<<A+1\nA", complete: true)
Result: "#{RED}<<A#{CLEAR}+1\n#{RED}A#{CLEAR}"
<"\e[31m<<A\e[0m+\e[34m\e[1m1\e[0m\n" + "\e[31mA\e[0m"> expected but was
<"\e[31m<<A\e[0m+1\n" + "\e[31mA\e[0m">
diff:
? <<A+1m1
A
=========================================================================================================================================================================
...skip...
..........................
Finished in 4.4035 seconds.
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------
114 tests, 1103 assertions, 1 failures, 0 errors, 6 pendings, 0 omissions, 0 notifications
93.8596% passed
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------
25.89 tests/s, 250.48 assertions/s
rake aborted!
Command failed with status (1)
/Users/tomoya/.rbenv/versions/2.6.0/bin/bundle:23:in `load'
/Users/tomoya/.rbenv/versions/2.6.0/bin/bundle:23:in `<main>'
Tasks: TOP => test
(See full trace by running task with --trace)
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 sorry I missed that 🤦♂️
end | ||
end | ||
yield(nil, inner_code.byteslice(byte_pos...inner_code.bytesize), nil) if byte_pos < inner_code.bytesize |
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.
Can we add a comment to explain what cases this handles?
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 added comment yield uncolorable DATA section
a7efb93
to
da54e7f
Compare
@tompng thanks for your fix, I think this PR looks good 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.
Looks good. Thank you for your contribution!
Fixed
IRB::Color.colorize_code
to fix irb crash.irb crashes when I pasted the code below.
IRB::Color.colorize_code
did not colorized the DATA part, andReline.output_modifier_proc
returned too few lines, causedstring + nil
error.IRB::Color.colorize_code must colorize all lines, and IMO, all characters.
Ripper.lex sometimes skips some characters.
Since colorize_code is used in
ruby -run -e colorize
, ability to colorize every characters in a code that includes"\0"
"\4"
"\32"
might be useful.In this pull request, I changed
IRB::Color.scan
to also scan those skipped chars and include it in the colorized string.