-
Notifications
You must be signed in to change notification settings - Fork 141
Fix nil error on debugger prompt #1097
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
Conversation
lib/irb.rb
Outdated
| @context.irb_name | ||
| when "m" | ||
| main_str = @context.safe_method_call_on_main(:to_s) rescue "!#{$!.class}" | ||
| main_str = main_str || "!#{@context.safe_method_call_on_main(:class)}" |
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 think ! in "!RuntimeError" is expressing that stringifying main raised RuntimeError, not that main is a RuntimeError.
So we don't need ! prefixed to a non-error class name.
I think main_str can be just an empty string. Just like when to_s returned an empty string.
How about this?
main_str = "#{@context.safe_method_call_on_main(:to_s)}" rescue "!#{$!.class}"This way, it can handle other erroneous case such as to_s returned Object.new
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.
Or this one
main_str = "!TypeError" unless String === main_strThere 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.
Thank you for reviewing.
I misunderstood meaning of prompt.
And "#{@context.safe_method_call_on_main(:to_s)}" looks better!
So I changed.
| assert_match(/=> 2\| puts "hello"/, output) | ||
| end | ||
|
|
||
| def test_debug_class_to_s_return_string |
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 test case is not needed.
It is testing a common normal case that to_s is defined and it returns string. I think this case is already tested in another test case.
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 test case.
tompng
left a comment
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.
Thank you 👍
Fix #1089
This change ensures that when a class's to_s method returns nil, it is handled in the same way as if to_s had raised an error.