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.

Sign up for GitHub

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

Jump to bottom

Support frozen string literals #3

Merged
merged 2 commits into from Nov 13, 2017
Merged

Support frozen string literals #3

merged 2 commits into from Nov 13, 2017

Conversation

flavorjones
Copy link
Contributor

Related to flavorjones/loofah#118, I'd like to ensure that Loofah supports Ruby's "frozen string literals" option.

Essentially this PR turns instances of '' which are mutated into a call to String.new, which returns mutable strings. All changes were driven by failing tests when this option is set.

@flavorjones flavorjones mentioned this pull request Nov 13, 2017
Closed
@flavorjones flavorjones force-pushed the flavorjones-support-frozen-string-literals branch from 46f0146 to 67d41bc Compare November 13, 2017 15:14
@rgrove
Copy link
Owner

rgrove commented Nov 13, 2017

@flavorjones Thanks! Looks like tests are failing in older Rubies due to the unknown --enable-frozen-string-literal option though:

$ bundle exec rake
NOTE: Testing support for frozen string literals
/home/travis/.rvm/rubies/ruby-2.2.2/bin/ruby: warning: unknown argument for --enable: `frozen-string-literal'
/home/travis/.rvm/rubies/ruby-2.2.2/bin/ruby: invalid option --debug=frozen-string-literal  (-h will show valid options) (RuntimeError)
rake aborted!

@flavorjones
Copy link
Contributor Author

Ugh! Sorry about that. Fix coming ...

@flavorjones flavorjones force-pushed the flavorjones-support-frozen-string-literals branch from 67d41bc to 4d5ad71 Compare November 13, 2017 18:26
@flavorjones
Copy link
Contributor Author

All green!

@flavorjones
Copy link
Contributor Author

No, sorry, I messed that up. Hang on. Case of the mondays.

@flavorjones
ensure tests set --enable-frozen-string-literal
1e09263 
to prevent regressions in this support.
@flavorjones
use String.new instead of empty string constants
14016ae 
to support use of frozen string literals.
@flavorjones flavorjones force-pushed the flavorjones-support-frozen-string-literals branch from 4d5ad71 to 14016ae Compare November 13, 2017 18:38
@rgrove
Copy link
Owner

rgrove commented Nov 13, 2017

@flavorjones Looks like tests are passing now, thanks! Is this ready to merge or should I continue hanging on?

@flavorjones
Copy link
Contributor Author

I think you can :shipit: !

@rgrove rgrove merged commit 14016ae into rgrove:master Nov 13, 2017
@flavorjones flavorjones deleted the flavorjones-support-frozen-string-literals branch November 25, 2019 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@flavorjones @rgrove