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 ArgumentError on escaping frozen String #7

Merged
merged 1 commit into from Nov 22, 2018

Conversation

r7kamura
Copy link
Contributor

@r7kamura r7kamura commented Nov 22, 2018

Thank you @condor for creating this great gem! It's very convenient to format our application logs, but I have an idea that it would be even better if it supported frozen String values in its #dump implementation.

Problem

I use ltsv gem to format our chronological logs in our Rails application like this:

LTSV.dump(foo: "bar", time: Time.current)

Expected behavior

I think it should return a String like "foo:bar\ttime:2018-11-22 15:10:34 +0900" without any exception.

Actual behavior

But unfortunately the code above raises ArgumentError. This is because its implementation tries to mutate the given String by using String#gsub! to escape special characters.

Note: Time.current.to_s (ActiveSupport::TimeWithZone#to_s) returns a frozen String (since Rails 5.2.0), and LTSV#escape uses it.

ltsv/lib/ltsv.rb

Lines 134 to 139 in f545450

value = string.kind_of?(String) ? string.dup : string.to_s
value.gsub!("\\", "\\\\")
value.gsub!("\n", "\\n")
value.gsub!("\r", "\\r")
value.gsub!("\t", "\\t")

Solution

To fix the problem above, I replaced String#gsub! with String#gsub.

I want to think there is no special reason to mutate given String here, but I'm afraid there might be a performance reason to choose String#gsub!. What do you think @condor?

@condor condor merged commit 4227f44 into condor:master Nov 22, 2018
@r7kamura r7kamura deleted the feature/frozen branch November 22, 2018 06:36
@condor
Copy link
Owner

condor commented Nov 22, 2018

LGTM! Thanks for your contribution!

@r7kamura
Copy link
Contributor Author

Thank you for the so quick response!
I'm looking forward to the new version of ltsv gem (v0.1.1 or v0.2.0?) ✨ 💎 ✨

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.

None yet

2 participants