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

use frozen string literal for ruby 2.3.0+ #970

Merged
merged 1 commit into from
Mar 7, 2016

Conversation

twalpole
Copy link
Contributor

@twalpole twalpole commented Mar 3, 2016

No description provided.

def to_s(*args)
location.to_s
end

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Superfluous whitespace?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change removes the superfluous whitespace that was in the file (my editor did that I guess) -- if you're saying the line is unneeded, it was there before but with some spaces at the beginning

@jeremy
Copy link
Collaborator

jeremy commented Mar 5, 2016

Nice work, @twalpole! Good call including the pragma everywhere.

Some of these cases suggest that methods are mutating strings that they probably shouldn't.

String.new(…) makes a copy of the argument, so selective use of #dup may end up feeling cleaner. Having to shed our literal strings in so many places frequently feels like prematurely avoiding frozen strings.

value.sub! FILENAME_RE, '\1="\2"'
value.sub FILENAME_RE, '\1="\2"'
else
value
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@twalpole
Copy link
Contributor Author

twalpole commented Mar 7, 2016

@jeremy Ok -I've swapped to dup in most places and also cleaned up the raise_error warnings to make sure they weren't hiding frozen string errors.

@@ -71,6 +72,7 @@ def []=(name, value)

if hash[:body].respond_to? :force_encoding and hash[:body].respond_to? :valid_encoding?
if not hash[:body].valid_encoding? and default_values[:content_transfer_encoding].downcase == "binary"
hash[:body] = hash[:body].dup if hash[:body].frozen?
hash[:body].force_encoding("BINARY")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another spot to sidestep the frozen check:

hash[:body] = hash[:body].dup.force_encoding("BINARY")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is in the gems code as opposed to test, and I figured the body could be large -- for performance and memory reasons I didn't want to dup it unless necessary

@jeremy
Copy link
Collaborator

jeremy commented Mar 7, 2016

Looking good @twalpole!

Would you squash down to a single commit?

@twalpole
Copy link
Contributor Author

twalpole commented Mar 7, 2016

@jeremy done

@jeremy jeremy merged commit 5b71d55 into mikel:master Mar 7, 2016
jeremy added a commit that referenced this pull request Mar 7, 2016
use frozen string literal for ruby 2.3.0+
@aried3r
Copy link

aried3r commented Mar 8, 2016

Oh, that's great!

I guess that fixes #965 except the method I mentioned there also uses .force_encoding but was not modified in this PR. Is the input of the method now already "sanitized"?

@twalpole
Copy link
Contributor Author

twalpole commented Mar 8, 2016

@aried3r The one string quote_phrase is called with in the code (display_name) is now mutable so from that perspective it's fixed - If using the method from outside it still forces the encoding on the string and then forces it back to the original encoding - I'll do a quick PR for that

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.

3 participants