Skip to content

Add frozen string literal comment to all files#167

Merged
ssinghi merged 2 commits into
kreeti:masterfrom
tablecheck:frozen-string-literal
May 14, 2026
Merged

Add frozen string literal comment to all files#167
ssinghi merged 2 commits into
kreeti:masterfrom
tablecheck:frozen-string-literal

Conversation

@johnnyshields
Copy link
Copy Markdown

@johnnyshields johnnyshields commented Apr 11, 2026

This PR adds the frozen string literal comment to all files.

This is important so that Ruby apps using Paperclip and can enable frozen string literals everywhere in the future. Frozen strings everywhere are the future planned standard of Ruby.

@johnnyshields johnnyshields force-pushed the frozen-string-literal branch 5 times, most recently from 4d39e25 to c4ad2bd Compare April 12, 2026 12:27
@ssinghi
Copy link
Copy Markdown

ssinghi commented Apr 27, 2026

Hi @johnnyshields I have merged the PR #166, can you please update this PR.

@johnnyshields johnnyshields force-pushed the frozen-string-literal branch from 91f5495 to ccca0bd Compare May 4, 2026 08:45
@johnnyshields
Copy link
Copy Markdown
Author

@ssinghi PR updated, please merge

@ssinghi
Copy link
Copy Markdown

ssinghi commented May 5, 2026

I thought a bit on this, and I am not very keen on this PR as ruby 3.4 makes frozen_string_literal true by default.

@johnnyshields
Copy link
Copy Markdown
Author

johnnyshields commented May 6, 2026

@ssinghi

I am not very keen on this PR as ruby 3.4 makes frozen_string_literal true by default.

Your comment is incorrect. Neither Ruby 3.4 nor Ruby 4.0 makes frozen_string_literal true by default. The switch-over change is unlikely to land until Ruby 5.0 in a few years. You can see a discussion in the Ruby language issue tracker here: https://bugs.ruby-lang.org/issues/20205

But even if Ruby did set it to true, this PR would still be necessary, because the code in kt-paperclip currently breaks when you set it to true. That is the point of this PR. See for example this line. But it is not enough to fix these cases as a one-off; we must also the frozen string magic comment everywhere in the gem, to ensure it remains compliant even for future changes.

It is standard practice for all gems to add this frozen string magic comment, because it enables applications upgrade to use frozen strings by default everywhere. As per the Ruby discussion above, using frozen strings application-wide can result in a 5-10% speedup in real-world production applications.

Of the 460 gems bundled in my application, there were only two gems (kt-paperclip and one other) that were not frozen string compliant. You can see Rails, Puma, and basically all other mainstream gems are doing this, and it is also enforced by Rubocop's default rules.

@rocket-turtle
Copy link
Copy Markdown

I also think this should be merged. Its removing a lot of .rubocop_todo.yml lines.

@johnnyshields
Copy link
Copy Markdown
Author

FYI I've been running this branch in production for over a week. No issues.

@ssinghi ssinghi merged commit 10e3600 into kreeti:master May 14, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants