-
Notifications
You must be signed in to change notification settings - Fork 605
fix pry indent frozen error #2136
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 pry indent frozen error #2136
Conversation
lib/pry/indent.rb
Outdated
| def reset | ||
| @stack = [] | ||
| @indent_level = '' | ||
| @indent_level = ''.dup |
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.
Nice work finding and fixing this! I believe we can use String.new to create a mutable 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.
@kyrylo thanks for your feedback! Updated the part according to your advice.
For the initial value, I added an empty string '' to ensure that the encoding stays as UTF-8 (not ASCII-8bit).
(this doesn't make a big difference in the current code base, so I'm happy to modify it as String.new if that's your preference.)
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.
Thanks for changing. I think we should either change this to just String.new or add a test that verifies the encoding of the string is UTF-8. It looks rather confusing as of now. My preference is the first option since like you mentioned it doesn't make any difference.
|
@barrettkingram not really. Needs a small correction and we are good to go. I'll give this some time and if the PR author doesn't address the feedback, I'll merge as is and fix later. |
|
Gentle ping @wakasa51. It would be nice if you could be the author of this change :) |
|
@kyrylo Sorry, I didn't notice your message. |
This PR fixes #2135
I think there are multiple ways to tackle this, for instance
subinstead ofsub!(recreating another string)sub!For this PR I took the latter approach but I'm happy to know if the earlier makes better sense to you.