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

Add frozen_string_literal everywhere and fix string mutation with force_encoding #592

Conversation

AnotherJoSmith
Copy link

@AnotherJoSmith AnotherJoSmith commented May 30, 2018

This PR has two commits:

  1. Add the magic comment # frozen_string_literal: true to every file. This will help with migration to ruby 3, where all literals will be frozen. This functionality was added in ruby 2.3.
  2. Duplicate strings before calling force_encoding in Request. String#force_encoding is a mutating function, which will raise a RuntimeError if the response body is a frozen string.

This is mostly causing issues for us with testing using stub_response in our application, since we have enabled frozen string literals.

For example, the following test fails when frozen_string_literals are enabled:

it "should assume utf-16 little endian if options has been chosen" do
@request.options[:assume_utf16_is_big_endian] = false
response = stub_response "C\x00o\x00n\x00t\x00e\x00n\x00t\x00"
response.initialize_http_header("Content-Type" => "text/plain;charset = utf-16")
resp = @request.perform
expect(response_charset).to_not be_empty
expect(resp.body.encoding).to eq(Encoding.find("UTF-16LE"))
end

The issue also exists with this gem's tests, which you can see by running git checkout 9c4c7a0 && bundle exec rake.

This should not be a breaking change and is being done by other gems as well (e.g. stripe/stripe-ruby#649).

@jnunemaker If you're not interested adding the frozen_string_literal comment everywhere, I'd be happy to pull out 1f29818 as a separate PR as well.

This special comment is helpful to prepare for ruby 3, where string
literals will be frozen by default.
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.

1 participant