-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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.
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
[Tooling/Fastlane] Delete unused helpers #13965
Conversation
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
You can test the changes on this Pull Request by downloading the APK here. |
76060b5
to
e0c6052
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing android_git_helper
and android_version_helper
look good to me. I briefly tested it on a modified version of new_beta_release
and it worked as expected.
I can't find android_virtual_device_helper.rb
in release-toolkit
tho 🤔 Am I blind or missing something?
It's not, but iirc it's also not used anywhere (at least I couldn't find a reference to any of the methods in that helper anywhere in WPiOS, but would love a confidence check on that too) |
It looks like it's used in |
e0c6052
to
9ba4604
Compare
Ah! You're right indeed! Not sure how I missed it (my guess is because there was multiple modules in that file, which is unusual and not idiomatic in Ruby, I might have only searched for references of the first one… 🤷 )… thank god for peer reviews! 😉 Fixed, so we should be all good now! |
Note: the Detekt linter failure is not coming from this PR, and in fact also fails in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is ok in my view – I've verified both these helpers are now present over in the release toolkit (and freshly refactored!)
IMHO we can leave it to @JavonDavis to press merge on this, since he'd be the one dealing with any breakage 😁
(including when we open just the fastlane/ subfolder in RubyMine…)
9ba4604
to
c51f2e3
Compare
Generated by 🚫 dangerJS |
I've rebased the branch on top of @JavonDavis please merge whenever you're ready (and CI is green) 🙂 – preferably before next release so you can test the changes on it |
Part of wordpress-mobile/release-toolkit#204
This only removes the helpers that were either unused or are already present in release-toolkit.
Does not seem like much of a diff, but that change required me to compare each helper from this repo with the helpers from release-toolkit to ensure that even if they diverged over time and the diff wasn't empty, removing the local copies would not break anything.
For this first step I let the helpers and actions related to screenshots as they seem to only be present in WPAndroid. A separate step / PR will move those remaining ones into the release-toolkit and then remove them from the WPAndroid repo, but that's a larger piece of work (especially for screenshots we still need repo-specific files locally, like the
.ini
files, so the split won't be that trivial if we want it to be flexible and adapt to multiple repos in the future)To test: