-
-
Notifications
You must be signed in to change notification settings - Fork 572
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
Enable frozen string literal #1039
Enable frozen string literal #1039
Conversation
8de406f
to
0ea1814
Compare
33002a6
to
5c89ff8
Compare
I'm not sure how to deal with the newer jruby version warning causing the tests to fail. Any input would be welcome |
c78376c
to
e5e2b84
Compare
e7f8ae4
to
37ae2e4
Compare
Ooff, that's annoying. |
@@ -213,7 +213,7 @@ def unescape_interpolation(str, escape_html = nil) | |||
scan.scan(/\w+/) | |||
end | |||
content = eval("\"#{interpolated}\"") | |||
content.prepend(char) if char == '@' || char == '$' | |||
content = "#{char}#{content}" if char == '@' || char == '$' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be curious if there are any performance issues here...?
@@ -37,7 +37,7 @@ def inspect | |||
print Haml::Engine.new('%div{ foo: true, bar: false }').render | |||
HAML | |||
f.close | |||
out = IO.popen([RbConfig.ruby, f.path], &:read) | |||
out = IO.popen([RbConfig.ruby, '-W0', f.path], &:read) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a documentation note here on why the -W0
is there and what it's used for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here the I attempted to set ruby interpreter warning level to 0 (=silence), with hopes that jruby warning wouldn't be printed out. I see that you've already merged the PR, I guess, I can leave it as is.
@@ -160,7 +160,7 @@ def test_silent_end_with_stuff | |||
b | |||
a | |||
HTML | |||
- str = "abcde" | |||
- str = +"abcde" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, I had no idea about this feature... it took me some serious digging to learn what it even was. Great work!
Ran the tests locally with the same version of JRuby and it passed just fine... still investiating. |
Since the intent of the breaking test is to capture an interesting edge case, I have slightly widened it's expectations, so that we don't have to be 100% sure that ALL interpreters NEVER print a warning when executed as a child or in various different configurations. |
Further, this fails on the |
#967 added the frozen string literal magic comments on all ruby files.
To ensure that the strings are not modified in the future, I think the tests also should cover it. This PR fixes a string mutation case and runs tests with
RUBYOPT=--enable-frozen-string-literal
, where the ruby interpreter will ensure the frozen strings.