-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -128,10 +128,14 @@ def colorize_code(code, complete: true, ignore_error: false, colorable: colorabl | |
|
||
symbol_state = SymbolState.new | ||
colored = +'' | ||
length = 0 | ||
end_seen = false | ||
|
||
scan(code, allow_last_error: !complete) do |token, str, expr| | ||
# handle uncolorable code | ||
if token.nil? | ||
colored << Reline::Unicode.escape_for_print(str) | ||
next | ||
end | ||
|
||
# IRB::ColorPrinter skips colorizing fragments with any invalid token | ||
if ignore_error && ERROR_TOKENS.include?(token) | ||
return Reline::Unicode.escape_for_print(code) | ||
|
@@ -147,15 +151,7 @@ def colorize_code(code, complete: true, ignore_error: false, colorable: colorabl | |
colored << line | ||
end | ||
end | ||
length += str.bytesize | ||
end_seen = true if token == :on___end__ | ||
end | ||
|
||
# give up colorizing incomplete Ripper tokens | ||
unless end_seen or length == code.bytesize | ||
return Reline::Unicode.escape_for_print(code) | ||
end | ||
|
||
colored | ||
end | ||
|
||
|
@@ -170,33 +166,42 @@ def without_circular_ref(obj, seen:, &block) | |
end | ||
|
||
def scan(code, allow_last_error:) | ||
pos = [1, 0] | ||
|
||
verbose, $VERBOSE = $VERBOSE, nil | ||
RubyLex.compile_with_errors_suppressed(code) do |inner_code, line_no| | ||
lexer = Ripper::Lexer.new(inner_code, '(ripper)', line_no) | ||
if lexer.respond_to?(:scan) # Ruby 2.7+ | ||
lexer.scan.each do |elem| | ||
str = elem.tok | ||
next if allow_last_error and /meets end of file|unexpected end-of-input/ =~ elem.message | ||
next if ([elem.pos[0], elem.pos[1] + str.bytesize] <=> pos) <= 0 | ||
byte_pos = 0 | ||
line_positions = [0] | ||
inner_code.lines.each do |line| | ||
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 commentThe 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:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The code you suggested looks more readable. thanks |
||
start_pos = line_positions[elem.pos[0] - 1] + elem.pos[1] | ||
|
||
str.each_line do |line| | ||
if line.end_with?("\n") | ||
pos[0] += 1 | ||
pos[1] = 0 | ||
else | ||
pos[1] += line.bytesize | ||
end | ||
end | ||
# 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 | ||
|
||
if lexer.respond_to?(:scan) # Ruby 2.7+ | ||
lexer.scan.each do |elem| | ||
next if allow_last_error and /meets end of file|unexpected end-of-input/ =~ elem.message | ||
on_scan.call(elem) | ||
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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. I added test case Another code There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I removed this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah sorry I missed that 🤦♂️ |
||
on_scan.call(elem) | ||
end | ||
end | ||
# yield uncolorable DATA section | ||
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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. I added comment |
||
end | ||
ensure | ||
$VERBOSE = verbose | ||
|
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 toReline::Unicode.escape_for_print
at line145
and then be pushed tocolored
at line150
. 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)
anddispatch_seq
is originally designed to receiveRipper::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 👍