Rename a few metadata update helper classes

[crazytonyli]

Issue

A few classes (Fastlane::Helper::MetadataBlock & co.) with the same names are defined in two source files: metadata_update_helper.rb and an_metadata_update_helper.rb. That means when you use those classes, there really isn't any guarantee which implementation you'll get at runtime.

Based on this require an_metadata_update_helper.rb statement, I think we would want to use the classes defined in an_metadata_update_helper.rb in the an_update_metadata_source fastlane action. But when running in Ruby 3, the classes (for example Fastlane::Helper::UnknownMetadataBlock) used in that action aren't the ones defined in the an_metadata_update_helper.rb file.

You can reproduce this issue by inspect the class's source code location. First, make the following code changes locally.

diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/actions/android/an_update_metadata_source_action.rb b/lib/fastlane/plugin/wpmreleasetoolkit/actions/android/an_update_metadata_source_action.rb
index 7f2e7cc8..ae08b860 100644
--- a/lib/fastlane/plugin/wpmreleasetoolkit/actions/android/an_update_metadata_source_action.rb
+++ b/lib/fastlane/plugin/wpmreleasetoolkit/actions/android/an_update_metadata_source_action.rb
@@ -5,6 +5,9 @@ module Fastlane
   module Actions
     class AnUpdateMetadataSourceAction < Action
       def self.run(params)
+        puts "Ruby version: #{RUBY_VERSION}"
+        puts Fastlane::Helper::UnknownMetadataBlock.new.method(:initialize).source_location.join('#')
+        exit 1
         # fastlane will take care of reading in the parameter and fetching the environment variable:
         UI.message "Parameter .po file path: #{params[:po_file_path]}"
         UI.message "Release version: #{params[:release_version]}"

Check out the trunk branch and run bundle exec fastlane run an_update_metadata_source. You should see a similar output like this:

[12:04:24]: ---------------------------------------
[12:04:24]: --- Step: an_update_metadata_source ---
[12:04:24]: ---------------------------------------
Ruby version: 2.7.4
/Users/tonyli/Projects/release-toolkit/lib/fastlane/plugin/wpmreleasetoolkit/helper/an_metadata_update_helper.rb#23

Checkout branch use-ruby-3.2.2 and run the same command. You should see output like this:

[12:05:15]: ---------------------------------------
[12:05:15]: --- Step: an_update_metadata_source ---
[12:05:15]: ---------------------------------------
Ruby version: 3.2.2
/Users/tonyli/Projects/release-toolkit/lib/fastlane/plugin/wpmreleasetoolkit/helper/metadata_update_helper.rb#23

The result is, the code uses the same class, but different implementations get picked up at runtime.

Solution

There are probably some historical issues underneath that I'm not aware of. And based on some pending unit tests, there probably was a plan to address those issues.

This PR doesn't intend to address all the issues hidden behind the duplicated symbols though. This PR simply wants to fix the duplicated symbols, by adding an An prefix to the classes defined in the an_metadata_update_helper.rb file.

I have made a major assumption here. That is the an_update_metadata_source fastlane action is the only one that uses the classes in an_metadata_update_helper.rb, which is why I only changed that one source file. It would be good to get a confirmation whether this assumption is correct.

Failing unit tests

One unit test doesn't agree with my changes. There are two unexpected failures in ios_update_metadata_source_spec.rb and gp_update_metadata_source_spec.rb (the failing test case is in the shared_examples_for_update_metadata_source_action.rb file.)

I don't think they fail because the changes are incorrect. My assumption is those unit tests were written incorrectly. You can do a similar test as above. First, making the following code changes:

diff --git a/spec/ios_update_metadata_source_spec.rb b/spec/ios_update_metadata_source_spec.rb
index f4c33b34..ca0a9f50 100644
--- a/spec/ios_update_metadata_source_spec.rb
+++ b/spec/ios_update_metadata_source_spec.rb
@@ -14,5 +14,9 @@ describe Fastlane::Actions::IosUpdateMetadataSourceAction do
     allow(Fastlane::Actions::EnsureGitStatusCleanAction).to receive(:run)
   end
 
+  puts "Ruby version: #{RUBY_VERSION}"
+  puts Fastlane::Helper::UnknownMetadataBlock.new.method(:initialize).source_location.join('#')
+  exit 1
+
   include_examples 'update_metadata_source_action', whats_new_fails: false
 end

Checkout the trunk branch and run bundle exec rspec ./spec/ios_update_metadata_source_spec.rb. You should see output like this:

Ruby version: 2.7.4
/Users/tonyli/Projects/release-toolkit/lib/fastlane/plugin/wpmreleasetoolkit/helper/an_metadata_update_helper.rb#23

That is obviously incorrect. The test case runs tests against IosUpdateMetadataSourceAction, but with a class defined in an_ metadata_update_helper.rb.

What I did was moving that failing test case to the Fastlane::Actions::AnUpdateMetadataSourceAction test case, because maybe the expectations in it don't apply to the ios_ and gp_ ones. Again, I'm not familiar with po file generation logic. It'd be good to get a confirmation that this change is okay.

Test Instructions

I'm not entirely sure how to test this, as I don't know much about what is a "po" file and how it should be generated. I'm happy to carry out some testing if you have any suggestions.

Question

Should this change be a breaking change? The class name changes are a breaking changes, but I'm not sure if they are used outside of this library. The apps should use the actions, not the helper classes, correct?

Checklist before requesting a review

• Run bundle exec rubocop to test for code style violations and recommendations
• Add Unit Tests (aka specs/*_spec.rb) if applicable
• Run bundle exec rspec to run the whole test suite and ensure all your tests pass
• Make sure you added an entry in the CHANGELOG.md file to describe your changes under the appropriate existing ### subsection of the existing ## Trunk section.
• If applicable, add an entry in the MIGRATION.md file to describe how the changes will affect the migration from the previous major version and what the clients will need to change and consider.

[AliSoftware]

This PR doesn't intend to address all the issues hidden behind the duplicated symbols though. This PR simply wants to fix the duplicated symbols, by adding an An prefix to the classes defined in the an_metadata_update_helper.rb file.

That's an understandable goal 👍

But tbh iirc I think both metadata_update_helper.rb and an_metadata_update_helper.rb were historically the same thing (like, one started in the release-toolkit and the other implemented in a local action in the fastlane/actions/ folder of a specific client repo, until it got moved to release-toolkit) and thus should theoretically only diverge slightly (from having been updated separately) while they should really instead be a single action and helper.

You can find some more context for all of those in @mokagio post from last year about the state of those classes/actions [paaHJt-3fL-p2].

Given this, I wonder if we shouldn't use the opportunity of this PR to remove all of the implementations of this (in release-toolkit + any local implementation that might still be remaining in WPAndroid/WCAndroid repos) but one, and only keep the most recent one in release-toolkit (likely metadata_update_helper), like Gio suggested back then in paaHJt-3fL-p2#comment-5988. Especially since, if some of our repos still have that implemented locally in their own fastlane/actions/*rb, we can still have inconsistencies in which class gets used at runtime… even if you do the renaming in release-toolkit like you did here 😒 .

(PS: See also the RFC for our future plans about replacing this action with a gettext ruby lib [paaHJt-3gp-p2], but that plan is definitively for later, but some cleanup of those *metadata_update_helper.rb classes being duplicated in various places will still avoid runtime confusion in the meantime.)

[crazytonyli]

@AliSoftware That all sounds reasonable to me. I tried to look into the history of the localization project, but got overwhelmed by the amount of the information and unfamiliar territory like GlotPress. I think @mokagio has started to consolidate the two helper classes in #498. Beside, I think the PR—not having duplicated symbols—is an improvement on itself, right? We can resolve the duplicated symbols first, whilst looking into consolidating the two helpers.