combine_pdf
combine_pdf copied to clipboard
Fix `FrozenError`s
.dup
before .force_encoding
Calling .force_encoding
on a frozen String raises a 'FrozenError:
can't modify frozen String'.
This can be mitigated by making a copy before calling
.force_encoding
on that String.
:+1: We might as well add frozen_string_literal: false
to all the *.rb
files now, unless there are objections
If you mean # frozen_string_literal: true
, then yes.
Hi @bforma and @jethrodaniel ,
Thank you for your input and for the PR.
I love the discussion and agree that using frozen_string_literal: false
could work as an interim solution while code is being sanitized against frozen literals.
However, I'm super busy until December 25th and I'm not sure when I could test this approach and merge the changes.
I'll keep observing for now and hope for beautiful things :-)
Kindly, Bo.
@bforma ,
Could you state which Ruby version you're experiencing this issue with?
I don't get it on my machine.
Maybe you could author a quick Ruby example to reproduce the issue?
Thanks! Bo.
We are running with Ruby 2.6.5.
All of our Ruby source files have the # frozen_string_literal: true
magic comment.
Some of our tests failed due to a FrozenError: can't modify frozen String
.
I just noticed that our failing tests define a String
literal and (indirectly) pass that to CombinePDF.parse
. Just doing a .dup
on those String
s in the tests already fix the issue.
Perhaps this PR is not needed?
I just noticed that our failing tests define a String literal and (indirectly) pass that to CombinePDF.parse. Just doing a .dup on those Strings in the tests already fix the issue.
Perhaps this PR is not needed?
Perhaps it isn't needed in the short term, but it does shine a light on an issue with literals in the source code.
The source code could benefit from caching all the literals in binary form and then using them as needed. This could minimize object creation and improve performance for long running applications.