Skip to content

Add frozen_string_literal: false to files that modify strings and run tests with frozen strings #640

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
davidgm0 wants to merge 2 commits into from
Closed

Add frozen_string_literal: false to files that modify strings and run tests with frozen strings #640

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
be0834d
Run tests with frozen strings
davidgm0 Jun 4, 2025
2acdd1b
Add frozen_string_literal: false
davidgm0 Jun 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/workflows/integration-compute-core.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ concurrency:
jobs:
test-compute:
runs-on: fog-arc-runner
env:
RUBYOPT: "--enable-frozen-string-literal"
strategy:
matrix:
ruby-version: [ '3.0', '3.1', '3.2' ]
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/integration-compute-instance_groups.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ concurrency:
jobs:
test:
runs-on: fog-arc-runner
env:
RUBYOPT: "--enable-frozen-string-literal"
strategy:
matrix:
ruby-version: [ '3.0', '3.1', '3.2' ]
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/integration-compute-loadbalancing.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ concurrency:
jobs:
test:
runs-on: fog-arc-runner
env:
RUBYOPT: "--enable-frozen-string-literal"
strategy:
matrix:
ruby-version: [ '3.0', '3.1', '3.2' ]
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/integration-compute-networking.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ concurrency:
jobs:
test:
runs-on: fog-arc-runner
env:
RUBYOPT: "--enable-frozen-string-literal"
strategy:
matrix:
ruby-version: [ '3.0', '3.1', '3.2' ]
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/integration-monitoring.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ concurrency:
jobs:
test:
runs-on: fog-arc-runner
env:
RUBYOPT: "--enable-frozen-string-literal"
strategy:
matrix:
ruby-version: [ '3.0', '3.1', '3.2' ]
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/integration-pubsub.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ concurrency:
jobs:
test:
runs-on: fog-arc-runner
env:
RUBYOPT: "--enable-frozen-string-literal"
strategy:
matrix:
ruby-version: [ '3.0', '3.1', '3.2' ]
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/integration-sql.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ concurrency:
jobs:
test:
runs-on: fog-arc-runner
env:
RUBYOPT: "--enable-frozen-string-literal"
strategy:
matrix:
ruby-version: [ '3.0', '3.1', '3.2' ]
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/integration-storage.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ concurrency:
jobs:
test:
runs-on: fog-arc-runner
env:
RUBYOPT: "--enable-frozen-string-literal"
Copy link
Contributor

@stanhu stanhu Aug 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't we expect these tests to fail without fixes for the frozen strings?

Copy link
Author

@davidgm0 davidgm0 Aug 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To those files that modify strings I added:

# frozen_string_literal: false

That overrides the RUBYOPT for those files. The final behaviour should be that by default for all files the strings are frozen and for those few files the string literals are not frozen.

Copy link
Contributor

@stanhu stanhu Aug 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, right, that's unexpected, though; usually I see # frozen_string_literal: true, or it's omitted entirely because false is the default.

I'm not opposed to this approach as a step 1. However, at the moment these integration tests need to be run by hand. I suspect just fixing the frozen string issue is relatively straightforward that we might as well try to fix them on a file-by-file basis, such as:

diff --git a/lib/fog/google/storage/storage_json/real.rb b/lib/fog/google/storage/storage_json/real.rb
index 23c86c5..ab99372 100644
--- a/lib/fog/google/storage/storage_json/real.rb
+++ b/lib/fog/google/storage/storage_json/real.rb
@@ -45,7 +45,7 @@ DATA
           end
           string_to_sign << canonical_google_headers.to_s
 
-          canonical_resource = "/"
+          canonical_resource = +"/"
           if subdomain = params.delete(:subdomain)
             canonical_resource << "#{CGI.escape(subdomain).downcase}/"
           end
diff --git a/lib/fog/google/storage/storage_xml/real.rb b/lib/fog/google/storage/storage_xml/real.rb
index 166e181..5c15bfc 100644
--- a/lib/fog/google/storage/storage_xml/real.rb
+++ b/lib/fog/google/storage/storage_xml/real.rb
@@ -57,7 +57,7 @@ DATA
           end
           string_to_sign << canonical_google_headers.to_s
 
-          canonical_resource = "/"
+          canonical_resource = +"/"
           if subdomain = params.delete(:subdomain)
             canonical_resource << "#{CGI.escape(subdomain).downcase}/"
           end

Copy link

@dgmora dgmora Aug 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be happy if these changes could be applied directly, I just don't know the codebase so well and I saw there were a fair amount of modifications, so I went for the easy fix. That can still be applied after this PR.

Oh, right, that's unexpected, though; usually I see # frozen_string_literal: true, or it's omitted entirely because false is the default.

Yeah, but that default will eventually change. If someone tries to already enable --enable-frozen-string-literal (as I did), they will get errors. This PR at least fixes that

Copy link
Member

@geemus geemus Sep 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, using +"..." was how we solved this for excon and it worked well (though it is a bit annoying initially).

strategy:
matrix:
ruby-version: [ '3.0', '3.1', '3.2' ]
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/unit.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ concurrency:
jobs:
test-unit:
runs-on: ubuntu-latest
env:
RUBYOPT: "--enable-frozen-string-literal"
strategy:
matrix:
ruby-version: [ '3.0', '3.1', '3.2', 'head', 'truffleruby-head']
Expand Down
4 changes: 3 additions & 1 deletion .rubocop.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@ Style/FormatString:
Style/RegexpLiteral:
Enabled: false

#TODO: this needs to be adressed not through the linter
# TODO: Some files modify strings in place, so we need to disable this cop.
# ci runs with RUBYOPT="--enable-frozen-string-literal", files that modify
# strings have frozen_string_literal: false.
Style/FrozenStringLiteralComment:
Enabled: false

Expand Down
2 changes: 2 additions & 0 deletions lib/fog/google/storage/storage_json/real.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
# frozen_string_literal: false

module Fog
module Google
class StorageJSON
Expand Down
2 changes: 2 additions & 0 deletions lib/fog/google/storage/storage_json/utils.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
# frozen_string_literal: false

require "addressable"

module Fog
Expand Down
2 changes: 2 additions & 0 deletions lib/fog/google/storage/storage_xml/real.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
# frozen_string_literal: false

module Fog
module Google
class StorageXML
Expand Down
2 changes: 2 additions & 0 deletions lib/fog/google/storage/storage_xml/utils.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
# frozen_string_literal: false

module Fog
module Google
class StorageXML
Expand Down