combine_pdf icon indicating copy to clipboard operation
combine_pdf copied to clipboard

Fix `FrozenError`s

Open bforma opened this issue 5 years ago • 7 comments

.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.

bforma avatar Nov 26 '19 15:11 bforma

:+1: We might as well add frozen_string_literal: false to all the *.rb files now, unless there are objections

jethrodaniel avatar Nov 26 '19 18:11 jethrodaniel

If you mean # frozen_string_literal: true, then yes.

bforma avatar Nov 26 '19 21:11 bforma

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.

boazsegev avatar Nov 27 '19 00:11 boazsegev

@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.

boazsegev avatar Nov 27 '19 00:11 boazsegev

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.

bforma avatar Nov 28 '19 09:11 bforma

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?

bforma avatar Nov 28 '19 09:11 bforma

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.

boazsegev avatar Nov 28 '19 13:11 boazsegev