Conversation
mokagio
left a comment
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 🤔
There was a problem hiding this comment.
...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'There was a problem hiding this comment.
Update: removed activesupport as a runtime dependency and added it only as a dev dependency: 0bc6274
I also had to add gem upper bound to avoid warning about the open-ended >= dependency
lib/fastlane/plugin/wpmreleasetoolkit/actions/common/promo_screenshots_action.rb
Outdated
Show resolved
Hide resolved
| # `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' |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Ah yes, maybe good to add a comment here as well: cocoapods enforces < 8.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
Co-authored-by: Olivier Halligon <olivier.halligon@automattic.com>
Summary
activesupportas a runtime dependency. The only production usage wasdeep_dupinPromoScreenshotsAction, replaced withMarshal.load(Marshal.dump(...)). Moved to a dev dependency (~> 7.2) for specs that use ActiveSupport methods (Time.use_zone,String#to_time,Hash#slice!).Style/FileOpen(useFile.foreachinstead ofFile.openwithout a block), fix obsoleteNaming/PredicateName→Naming/PredicatePrefixrename..rubocop_todo.ymlto.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 failuresbundle exec rubocop— no offensesMarshal.load(Marshal.dump(entry))behaves identically todeep_dupfor the hash structures used inPromoScreenshotsAction