Immutable strings #38
Conversation
spec/custom_tags_spec.rb
Outdated
| html << %Q{ </a>\n} | ||
| html << %Q{ <p>Figure #{label}</p>\n} | ||
| html << %Q{<div>\n} | ||
| html += %Q{ <a class="fig" href="/images/#{img}">\n} |
There was a problem hiding this comment.
Better to make the initial string mutable (using .dup at the end), and then keeping all the << calls. Using += means extra string allocations that aren't really necessary.
There was a problem hiding this comment.
That makes sense. It makes the diff smaller too.
spec/parser_spec.rb
Outdated
| output = RedCloth.new(input).to_html | ||
|
|
||
| output.should == "<p>This is an <span class=\"caps\">ISO</span>-8859-1 string</p>" | ||
| output.encoding.to_s.should == "ISO-8859-1" | ||
| end | ||
|
|
||
| it "should not raise ArgumentError: invalid byte sequence" do | ||
| s = "\xa3" | ||
| s.force_encoding 'iso-8859-1' | ||
| if RUBY_VERSION > "2.3.0" |
There was a problem hiding this comment.
Rather than having version-specific logic here, I'd recommend just making the initial string mutable: s = "\xa3".dup
lib/redcloth/formatters/base.rb
Outdated
| @@ -37,10 +36,10 @@ def pba(opts) | |||
| [:"padding-right", :"padding-left"].each do |a| | |||
| opts[:style] = "#{a}:#{opts[a]}em;#{opts[:style]}" if opts[a] | |||
| end | |||
| [:style, :class, :lang, :id, :colspan, :rowspan, :title, :start, :align].each do |a| | |||
| atts << " #{a}=\"#{ html_esc(opts[a].to_s, :html_escape_attributes) }\"" if opts[a] | |||
| atts = [:style, :class, :lang, :id, :colspan, :rowspan, :title, :start, :align].map do |a| | |||
There was a problem hiding this comment.
I've no strong preference here - perhaps the maintainers do - but I'd probably just make the definition of atts in the original version mutable with atts = ''.dup. Much of a muchness when it comes to the number of object allocations though 🤷♂️
|
Hi, thank you for your contribution. I'll be happy to merge this in once the CI build is fixed. |
The .load_documents method is deprecated and seems to have been removed entirely in the upcoming Ruby 2.5.
|
@joshuasiler I added a commit that fixes the build failure on ruby-head. |
|
@joshuasiler can you take another look please? |
|
Thanks, @joshuasiler! |
|
Awesome! :) A new gem release would be very welcome now... |
This PR fixes the cases where RedCloth doesn't work when string immutability is set.
Removes the line from Rakefile that sets RUBYOPT to allow setting string immutablity like so:
RUBYOPT="--enable-frozen-string-literal" be rake
(I kind of understand where this line came from but perhaps there is another way to achieve the same effect).
Updates the code of RedCloth and specs to not mutate string literals.
Background: This PR came from pry/pry#1625, via the build failures of coderay with string immutability set (https://travis-ci.org/rubychan/coderay/jobs/261625648).
I have not updated the Travis configuration to actually run with immutable strings, since tests will fail at the end due to RSpec mutating string literals.