[Tooling/Cleanup] Cleanup git helpers

[AliSoftware]

This PR cleans up all the ios_git_helper + android_git_helper + git_helper methods: refactoring + YARD doc.

Why?

• Many were duplicated in the platform-specific helper files while there was not reason not to DRY them in one common place
• Many methods in there were actually not really related to git. There were doing other actions then pushing the changes to git all in one place, which is probably why they ended up in those git_helpers, but most of the actions those helper were doing were something other than git in the first place
• Many methods didn't have very explicit name, and none of them were documented

How?

• Refactoring all the common methods into the common git_helper
• Cleaning up the calls to avoid repetition (many of those ended up just calling core methods like commit(message:files:push:)
• Update the call sites, and for some of the methods which were not that much related to git, move the atomic operations directly into the actions
• Added YARD documentation

To Review and Test

That's the trickiest part. This is very hard to test and we don't have rspec for most of those methods (see "Why not TDD" below) either.

The best we can do is:

• bundle exec rspec to ensure the existing tests still pass (even if there are not many tests covering the changes made there)
• be yard stats --list-undoc lib/fastlane/plugin/wpmreleasetoolkit/**/*git_helper.rb to check that everything about git helpers is documented (only items that are undocumented should be the Fastlane, Fastlane::Helper, and Fastlane::Helper::[Ios|Android] modules)
• bundle exec yard doc to generate the doc, then open yard-doc/index.html and inspect the documentation for the helpers
• Review the code manually to ensure I didn't introduce typos / syntax errors in the code and didn't miss any call sites when refactoring methods… things that we can only really check by proof-reading, given that Ruby is not type-checked… 😒
  • I split my progress in nice isolated commits, so you might prefer doing a per-commit review instead of the whole shebang.
• Optionally, point your Pluginfile to this branch and run a couple of actions (ideally the ones that would not risk having any side-effects) from an app repo to ensure those still work. Unfortunately, most of the actions and helpers impacted here do have side-effects, so that might be tricky…

Notes

Please do thorough review 🙏

This PR should be ready to be reviewed, but I pondered about opening it as Draft or not because I didn't proof-read the whole thing after I finished everything.

• I did proof-read my individual commits and double-checked their changes, but didn't proof-read the whole PR (as I use to do) now that all the dust have settled (also because it's 11pm and my brain is nothing but a blur now…).
• I didn't actually test any of the changes either, so many of those changes were done while flying blind… which doesn't inspire me much confidence… (That's the issue with a non-type-checked language that doesn't have unit tests 😓)

This means that, for example, a review directly by reading diff in GitHub might not be enough to ensure that I didn't forget to update all call sites… Which is why I'd appreciate a more thorough review of those changes, especially about ensuring that the behavior of helper methods are still unchanged (e.g. a parameter previously expecting just the version number for creating the release branch, but now expecting the full branch name or release: version instead…), there are no call sites I forgot to update, and there's no syntax error (e.g. missing comma) that slipped through…

(At some point I'd love to add rubocop to at least catch simple things like stupid syntax errors or a missing commas… but that's whole PR on its own…)

Work in Progress

Things that I will probably fix in a separate incremental PR (except if I get to address those before that Draft PR gets its final review…):

• There are still a couple of methods in the platform-specific git_helpers, especially the helper methods around metadata+localization, and the one helper method to commit files related to version bumps (which are different set of files depending on the platforms, but are still worth DRYing in a helper rather than moving in each action because there are multiple actions using them)
• In particular, the update_metadata methods (iOS+Android) and the localize_project method (iOS) are still calling non-git-related code (invoking the script).

Why not TDD?

I initially considered doing TDD and adding tests first, before refactoring. That's what tests are for after all.

Unfortunately, that ended up being very unproductive, given that the previous state of the code was really not easily testable and the amount of refactoring and thus the amount of methods (that I was gonna add tests for) that I was gonna delete, move and refactor entirely. That would have made any test written prior refactoring be obsolete by the end of it.

I know that it would have been ideal to write the tests prior to refactoring – that's the whole benefit of having tests after all – but that was too cumbersome to do, so I decided to keep the writing of the tests for a future PR addressing #193 .

[jkmassel]

A few nitpicks for your consideration and one very minor actual fix.

[jkmassel]

👍

[jkmassel]

Nice cleanup!

[jkmassel]

I like that this is now consolidated by its purpose, not it's use case – will definitely help reduce code and complexity!

[jkmassel]

Nitpick, but I wonder if it'd make sense to group the itc_ver and int_ver stuff together and only have one conditional? Might make it easier for future readers?

[AliSoftware]

Probably something I will pick up when I tackle #203

[codecov-io]

Codecov Report

Merging #206 (e34c727) into toolkit-cleanup (5891fe3) will increase coverage by 0.31%.
The diff coverage is 20.56%.

@@                 Coverage Diff                 @@
##           toolkit-cleanup     #206      +/-   ##
===================================================
+ Coverage            44.72%   45.04%   +0.31%     
===================================================
  Files                   93       93              
  Lines                 4013     3947      -66     
===================================================
- Hits                  1795     1778      -17     
+ Misses                2218     2169      -49     

Impacted Files	Coverage Δ
...kit/actions/android/android_betabuild_prechecks.rb	25.00% <0.00%> (ø)
...toolkit/actions/android/android_build_prechecks.rb	33.33% <0.00%> (ø)
...olkit/actions/android/android_bump_version_beta.rb	36.84% <0.00%> (ø)
...ions/android/android_bump_version_final_release.rb	33.33% <0.00%> (ø)
...kit/actions/android/android_bump_version_hotfix.rb	34.28% <0.00%> (ø)
...it/actions/android/android_bump_version_release.rb	30.00% <0.00%> (ø)
...it/actions/android/android_codefreeze_prechecks.rb	35.13% <0.00%> (ø)
...eleasetoolkit/actions/android/android_tag_build.rb	50.00% <0.00%> (-2.39%)	⬇️
...easetoolkit/actions/ios/ios_betabuild_prechecks.rb	30.76% <0.00%> (ø)
...eleasetoolkit/actions/ios/ios_bump_version_beta.rb	37.83% <0.00%> (ø)
... and 11 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5891fe3...e34c727. Read the comment docs.

[AliSoftware]

No idea why danger is suddenly failing in GitHub Actions?! 🧐
[EDIT] Just amended and force-pushed the last commit (with no change but its commit message) and it's green again 🤷

[jkmassel]

LGTM!

[mokagio]

I opened this but then realise I don't have enough time for a thorough review right now.

I just wanted to drop a not regarding "why not TDD?"

I know that it would have been ideal to write the tests prior to refactoring – that's the whole benefit of having tests after all – but that was too cumbersome to do, so I decided to keep the writing of the tests for a future PR addressing #193 .

No worries. All the work you're doing is improving this foundational codebase and making it easy to add tests later 👍

In app development, I'd say "Maybe write a few UI tests? Even if they hit the production APIs they'll still give you a bit of trust that nothing major broke during the refactor"

There could be a chance to do something similar here. Setup a test repo, install Fastlane, fetch this branch for the plugin, then write a few bash scripts that run the actions and check the how the repository changed.

Now that I said it out loud wrote it down, it sounds like a lot of work... Still, an idea worth putting out there. Maybe there's no need for it anymore or maybe there still is and that could help us in doing and testing the work. I don't know 🤷‍♂️

[AliSoftware]

@mokagio I'm merging this to progress on my dependant PR chain but don't hesitate to git it a review even post-merge if want and have time, I can always address any feedback in another PR down the chain ;)