Skip to content

Remove activesupport as a runtime dependency and clean up RuboCop config#709

Merged
iangmaia merged 8 commits intotrunkfrom
iangmaia/remove-activesupport-constraint
Mar 31, 2026
Merged

Remove activesupport as a runtime dependency and clean up RuboCop config#709
iangmaia merged 8 commits intotrunkfrom
iangmaia/remove-activesupport-constraint

Conversation

@iangmaia
Copy link
Copy Markdown
Contributor

@iangmaia iangmaia commented Mar 30, 2026

Summary

  • Remove activesupport as a runtime dependency. The only production usage was deep_dup in PromoScreenshotsAction, replaced with Marshal.load(Marshal.dump(...)). Moved to a dev dependency (~> 7.2) for specs that use ActiveSupport methods (Time.use_zone, String#to_time, Hash#slice!).
  • Fix RuboCop violations: resolve Style/FileOpen (use File.foreach instead of File.open without a block), fix obsolete Naming/PredicateNameNaming/PredicatePrefix rename.
  • Move permanent style choices from .rubocop_todo.yml to .rubocop.yml (e.g. Naming/PredicatePrefix, Naming/PredicateMethod, Style/Documentation, RSpec/MessageSpies, RSpec/SpecFilePathFormat, etc.), keeping only actionable tech debt in the todo file.

Test plan

  • bundle exec rspec — 800 examples, 0 failures
  • bundle exec rubocop — no offenses
  • Verified Marshal.load(Marshal.dump(entry)) behaves identically to deep_dup for the hash structures used in PromoScreenshotsAction

@iangmaia iangmaia self-assigned this Mar 30, 2026
@iangmaia iangmaia requested review from AliSoftware and mokagio March 30, 2026 22:20
Copy link
Copy Markdown
Contributor

@mokagio mokagio left a comment

Choose a reason for hiding this comment

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

Makes sense @iangmaia . Thanks for the ping.

I looked at the Git history and that upper bound was introduced to address a gem build warning:

WARNING:  open-ended dependency on activesupport (>= 6.1.7.1) is not recommended
  if activesupport is semantically versioned, use:
    add_runtime_dependency "activesupport", "~> 6.1", ">= 6.1.7.1"

Given there's no need to expose activesupport as an external dependency, the fix in this PR is better indeed.

Just in case, I run gem build from here and it gave no warnings.


Aside. activesupport comes as a CocoaPods dependency now. That made me realize that we are not that far from being able to remove CocoaPods itself from here. Without discarding the amazing service CP has done for us over the years, it will be a good day when we'll get rid of what is now essentially dead weight.

module SpecHelper
end

require 'active_support/all' # ActiveSupport methods (e.g. Time.use_zone, String#to_time) used by some specs
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This surprised me. I checked with GPT 5.4 and learned a bit about ActiveSupport's architecture and agreed that /all makes sense in the context of spec_helper.rb to simplify downstream usages.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

btw, this is also required on actions/common/promo_screenshots_action.rb. Given we still directly depend on it, maybe it makes sense to keep the explicit dependency and just remove the upper bound 🤔

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

...and removing the upper bound gives you this warning:

 WARNING:  open-ended dependency on activesupport (>= 6.1) is not recommended
       if activesupport is semantically versioned, use:
         add_runtime_dependency 'activesupport', '~> 6.1'

Copy link
Copy Markdown
Contributor Author

@iangmaia iangmaia Mar 31, 2026

Choose a reason for hiding this comment

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

I've now fixed some Rubocop violations and added a higher upper bound limit, and got no warnings on my side so I think we're good?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Update: removed activesupport as a runtime dependency and added it only as a dev dependency: 0bc6274

@iangmaia iangmaia marked this pull request as ready for review March 31, 2026 09:13
I also had to add gem upper bound to avoid warning about the open-ended >= dependency
@iangmaia iangmaia changed the title Remove activesupport < 8 version constraint from gemspec Update activesupport dependency and clean up RuboCop config Mar 31, 2026
@iangmaia iangmaia changed the title Update activesupport dependency and clean up RuboCop config Remove activesupport as a runtime dependency and clean up RuboCop config Mar 31, 2026
# `google-cloud-storage` is required by fastlane, but we pin it in case it's not in the future
spec.add_dependency 'google-cloud-storage', '~> 1.31'

spec.add_development_dependency 'activesupport', '~> 7.2'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

At that point I'm guessing it's probably OK to bump to 8.x when used only for running specs the risk is smaller than if it was used in prod code?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, maybe good to add a comment here as well: cocoapods enforces < 8.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Wait, do we need cocoapods as a dependency still? 🤔
(Maybe something to ponder for a follow-up PR though if you don't want to drag this PR too long)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hmm I assumed so, but a search shows ios_build_preflight.rb which calls other_action.cocoapods. So maybe we could remove it, as it seems we're using it via fastlane and not directly?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Okay 😄 2bc392c 🧹 🧹 🧹

iangmaia and others added 2 commits March 31, 2026 13:47
@iangmaia iangmaia merged commit fcf8e1a into trunk Mar 31, 2026
6 checks passed
@iangmaia iangmaia deleted the iangmaia/remove-activesupport-constraint branch March 31, 2026 14:43
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