Treat string literals as frozen#162
Conversation
aliismayilov
changed the title
aliismayilov
changed the title
aliismayilov
force-pushed
the
frozen-strings
branch
from
ea686c0 to
c2fabda
Compare
aliismayilov
force-pushed
the
frozen-strings
branch
from
98847af to
5453ef0
Compare
| - "RUBYOPT='-w' bundle exec rake $TASK" | ||
| - bundle exec rake $TASK | ||
| env: | ||
| - RUBYOPT='-w --enable-frozen-string-literal' |
There was a problem hiding this comment.
I'm fine with introducing frozen_string_literal: true and supporting --enable-frozen-string-literal with best-effort basis, however, I don't think it's a good idea to run CI with the non-default behavior. Besides, as long as we specify frozen_string_literal: true in every file, the behavior would be sufficiently tested anyway, at least inside Hamlit.
There was a problem hiding this comment.
Though, I'd be okay with adding the following config:
- rvm: 2.7.0
env: TASK=test RUBYOPT='-w --enable-frozen-string-literal'in the matrix.include if you like.
There was a problem hiding this comment.
The problem I see with frozen_string_literal: true is that in the long term the files will get moved around and new files will be introduced, and the comment might get forgotten.
I'd be happy with an extra job which runs the suite with --enable-frozen-string-literal option.
| @@ -1,3 +1,5 @@ | |||
| # frozen_string_literal: true | |||
|
|
|||
There was a problem hiding this comment.
Existing files don't put a space after the pragma.
| def find_and_preserve(input = nil, tags = haml_buffer.options[:preserve], &block) | ||
| return find_and_preserve(capture_haml(&block), input || tags) if block | ||
| tags = tags.each_with_object('') do |t, s| | ||
| tags = tags.each_with_object(+'') do |t, s| |
There was a problem hiding this comment.
This file is forked from Haml. The long-term idea is to carve out haml-parser.gem from haml.gem and share it between Haml and Hamlit, but it hasn't been done yet. So currently we're maintaining the files under lib/hamlit/parser/*.rb by basically copying Haml's files and replacing class names.
The original lib/haml/helpers.rb already supports frozen_string_literal: true, but in a different manner. To avoid further deviations, could you synchronize the implementation from https://github.com/haml/haml/blob/770962fec4cfe9ad2d9c6cc676b63d92932e7634/lib/haml/helpers.rb#L111-L119?
| static_attributes[name] = val | ||
| else | ||
| dynamic_attributes << "#{inspect_obj(name)} => #{val}," | ||
| dynamic_attributes += "#{inspect_obj(name)} => #{val}," |
| # `["Foo (Bar (Baz bang) bop)", " (Bang (bop bip))"]` in the example above. | ||
| def balance(scanner, start, finish, count = 0) | ||
| str = '' | ||
| str = +'' |
|
|
||
| def test_encoding_error # encoding | ||
| render("foo\nbar\nb\xFEaz".force_encoding("utf-8")) | ||
| render("foo\nbar\nb\xFEaz".encode("utf-8")) |
There was a problem hiding this comment.
same here https://github.com/haml/haml/blob/770962fec4cfe9ad2d9c6cc676b63d92932e7634/test/cases/encoding_test.rb#L73, this test file is also a fork.
|
To resolve most of the above problems without waiting for haml-parser.gem, I can work on automating Haml parser/test synchronization to this repository, which should be done regardless of this PR. If you agree with the above comments, I can take over this work from you. Please let me know if you like it. |
Yep, I agree with the comments. Feel free to take over :) |
aliismayilov
force-pushed
the
frozen-strings
branch
from
dff5cf2 to
c24150e
Compare
|
I completed the automation at #163 and added a minor modification to this branch. Now it's ready to be merged. Thank you! |
3 participants
Once this PR is merged, it should be "safe" to use this gem in a project with "RUBYOPT=--enable-frozen-string-literal", where the strings will be treated as frozen literals by default.