Skip to content
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

wakasa51
Copy link
Contributor

@wakasa51 wakasa51 commented May 31, 2020

This PR fixes #2135

I think there are multiple ways to tackle this, for instance

  • use sub instead of sub! (recreating another string)
  • don't use frozen string for sub!

For this PR I took the latter approach but I'm happy to know if the earlier makes better sense to you.

@@ -109,7 +109,7 @@ def initialize(pry_instance = Pry.new)
# reset internal state
def reset
@stack = []
@indent_level = ''
@indent_level = ''.dup
Copy link
Member

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.

Copy link
Contributor Author

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.)

Copy link
Member

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
Copy link
Contributor

@kyrylo Any blockers preventing this from getting merged? I tested this fix and confirmed it also resolves #2193 and #2195.

@kyrylo
Copy link
Member

kyrylo commented Jul 3, 2021

@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.

@kyrylo
Copy link
Member

kyrylo commented Jul 10, 2021

Gentle ping @wakasa51. It would be nice if you could be the author of this change :)

@wakasa51
Copy link
Contributor Author

wakasa51 commented Aug 3, 2021

@kyrylo Sorry, I didn't notice your message.
Thank you for the feedback! I have implemented it.

@wakasa51 wakasa51 requested a review from kyrylo October 10, 2021 10:09
@kyrylo kyrylo merged commit 5590e09 into pry:master Oct 11, 2021
@wakasa51 wakasa51 deleted the fix-frozen-error-if-midway-tokens-are-used-without-indentation branch October 18, 2021 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pry is terminated if MIDWAY_TOKENS are used without OPEN_TOKENS
3 participants