search

boazsegev / combine_pdf

A Pure ruby library to merge PDF files, number pages and maybe more...
MIT License
734 stars 156 forks source link

Fix `FrozenError`s #168

Open bforma opened 4 years ago

bforma commented 4 years ago

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

jethrodaniel commented 4 years ago

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

bforma commented 4 years ago

If you mean # frozen_string_literal: true, then yes.

boazsegev commented 4 years ago

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 commented 4 years ago

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

bforma commented 4 years ago

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 commented 4 years ago

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?

boazsegev commented 4 years ago

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.