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 for FrozenError when force_encode a frozen string literal #1479

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@programmarchy

programmarchy commented Sep 21, 2018

Issue #1478

@dentarg

This comment has been minimized.

Show comment
Hide comment
@dentarg

dentarg Sep 21, 2018

Is it possible to add a test for this?

dentarg commented Sep 21, 2018

Is it possible to add a test for this?

@programmarchy

This comment has been minimized.

Show comment
Hide comment
@programmarchy

programmarchy Sep 21, 2018

I think I've isolated a test case. I made a fix to my PR, and will work on adding a test as soon as I can.

programmarchy commented Sep 21, 2018

I think I've isolated a test case. I made a fix to my PR, and will work on adding a test as soon as I can.

@programmarchy

This comment has been minimized.

Show comment
Hide comment
@programmarchy

programmarchy Sep 22, 2018

Not all rubies have the unary + operator for strings. 🤕 So still need a way to unfreeze the string while keeping the same encoding and that's backwards compatible.

programmarchy commented Sep 22, 2018

Not all rubies have the unary + operator for strings. 🤕 So still need a way to unfreeze the string while keeping the same encoding and that's backwards compatible.

@programmarchy

This comment has been minimized.

Show comment
Hide comment
@programmarchy

programmarchy Sep 22, 2018

Kinda stuck here because not sure how to support ruby 2.2.9.

Using the unary operator (+data).force_encoding(encoding).encode! keeps the routing_tests happy because the encodings match what the specs expect. Ruby 2.2.9 does not support the unfreeze operator, though.

Alternatively, using data.dup.force_encoding(encoding).encode! causes the routing_tests to fail. Having a hard time understanding how those tests work, so can't debug -- can someone please take a look at why those tests fail on commit f016c24 (which uses dup)?

According to RuboCop, the unary operator behaves slightly differently than dup:

In Ruby 2.3 or later, use unary plus operator to unfreeze a string literal instead of String#dup and String.new. Unary plus operator is faster than String#dup.

Note: String.new (without operator) is not exactly the same as +''. These differ in encoding. String.new.encoding is always ASCII-8BIT. However, (+'').encoding is the same as script encoding(e.g. UTF-8). So, if you expect ASCII-8BIT encoding, disable this cop.

programmarchy commented Sep 22, 2018

Kinda stuck here because not sure how to support ruby 2.2.9.

Using the unary operator (+data).force_encoding(encoding).encode! keeps the routing_tests happy because the encodings match what the specs expect. Ruby 2.2.9 does not support the unfreeze operator, though.

Alternatively, using data.dup.force_encoding(encoding).encode! causes the routing_tests to fail. Having a hard time understanding how those tests work, so can't debug -- can someone please take a look at why those tests fail on commit f016c24 (which uses dup)?

According to RuboCop, the unary operator behaves slightly differently than dup:

In Ruby 2.3 or later, use unary plus operator to unfreeze a string literal instead of String#dup and String.new. Unary plus operator is faster than String#dup.

Note: String.new (without operator) is not exactly the same as +''. These differ in encoding. String.new.encoding is always ASCII-8BIT. However, (+'').encoding is the same as script encoding(e.g. UTF-8). So, if you expect ASCII-8BIT encoding, disable this cop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment