Remove duplicated Fastlane::Helper types

[mokagio]

Builds on top of #497 which inadvertently modified the ios_update_metadata_source action making it remove trailing \n in multi-line strings. See for example woocommerce/woocommerce-ios#10545.

Now, the question is why didn't the tests fail? 🤔 The reason is that we only had tests for the \n treatment behavior on what used to be StandardMetadataBlock and became AnStandardMetadataBlock with #497.

release-toolkit/spec/an_metadata_update_helper_spec.rb

Lines 4 to 32 in d30bc3a

	describe Fastlane::Helper::AnStandardMetadataBlock do
	it 'strips any trailing newline when generating the block for a single-line input' do
	Dir.mktmpdir do |dir|
	input = "Single line message with new line\n"
	# Generate the input file to convert to .pot block
	input_path = File.join(dir, 'input')
	File.write(input_path, input)
	# Ensure the input has only one line
	expect(File.read(input_path).lines.count).to eq 1
	# Write the .pot block in a StringIO to bypass the filesystem and have
	# faster tests
	output_io = StringIO.new
	described_class.new('any-key', input_path).generate_block(output_io)
	# Ensure the output matches the expectation: the trailing new line has been stripped.
	#
	# Note that the final new line is intentional. It's part of the formatting at the time of writing.
	expect(output_io.string).to eq <<~EXP
	msgctxt "any-key"
	msgid "Single line message with new line"
	msgstr ""
	EXP
	end
	end
	it 'does not strip a trailing new line when generating the block for a multi-line input' do

Everything had worked fine up till then because of the Ruby 2.7.4 behavior of always using the implementation that became An- for both iOS and Android actions. But after the implementations diverged in #497, the iOS behavior changed which we did not notice because there were no tests for the \n at the action level, only at the metadata block object.

This PR removes the duplication in the blocks and moves each in a dedicated file which I think keeps things tidier. The lack of action-level (integration?) tests lead us to this place, but I decided not to add tests at that level because we no longer have the confusing duplication.

I think the case could be made for focusing on action-level, integration tests for release-toolkit given that all its code is meant to be used via actions only. I think that's a separate discussion. I'd be happy to follow up this PR with more tests if that's the direction we decide to go.

Testing

This code has been used in Jetpack/WordPress and WooCommerce iOS PRs such as woocommerce/woocommerce-ios#10545. I also verified that no regression were introduced in Android locally by pointing WordPress Android to this branch and running the update_appstore_strings lane to confirm the .po files would not change:

Original description

Apart from the fact that it's confusing and redundant to have them, and that they've been a stone in our foot for a while, they result in runtime issues in Ruby 3.x.

@crazytonyli having a bit more context on the types touched by #497, I think we can safely remove the duplication. If I remember correctly, we didn't invest in that at the time under the assumption we'd soon rewrite the code, but that obviously hasn't happened. I think it's appropriate to address the issue now to avoid more confusion with the duplication later.

The test run and pass like this for me in Ruby 3.2.2, although I also had to apply other changes to the codebase.

Still in draft because I want to chase down the ios_ and gp_ unexpected behavior you mentioned in your PR.

What does it do?

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]

Super pedantic nitpic: all our other entries end each list item with a .

Suggested change

	- Fix metadata `po` generation for iOS projects removing the final `\n` [#498]
	- Fix metadata `po` generation for iOS projects removing the final `\n`. [#498]

[AliSoftware]

Damn auto-merge 😅

[mokagio]

I'll fix it as part of releasing a new version.

[AliSoftware]

At first I was confused as to why this PR updated the require statements of android/an_update_metadata_source_action.rb and common/gp_update_metadata_source.rb… but not ios/ios_update_metadata_source_action.rb

But then I saw that ios/ios_update_metadata_source_actions.rb's implementation is mostly calling other_action.gp_update_metadata_source under the hood 🤦
And in fact most of the glue code in android/an_update_metadata_source_action.rb can be found to be the same as common/gp_update_metadata_source… 🤦

Boy, what a mess those actions are… :sigh: 😒

I guess at some point it would make sense to also make an_update_metadata_source be just a wrapper over common/gp_update_metadata_source instead of repeating that implementation at that level too, so that both ios/ios_update_metadata_source_action and android/android_update_metadata_source_action use/wrap the same underlying gp_… action.

But that's probably for a separate PR, especially as the issue with the conflicting implementations of the models (the class …Block that you now moved into metadata/ subfolder 👍 ) has been the main offender and cause for all the trouble we've been having lately.
I'm sure there also are slight differences between the self.run and self.create_temp_po etc… implementation common/gp_…_source.rb and android/an_…_source.rb that would be worth merging too, but let's fix things one at a time.