Strip trailing new lines in single line `msgid` when generating `.po[t]` file #297

mokagio · 2021-08-24T06:30:54Z

At the moment, this is just a quick fix. After about an hour of puts-driven debugging against Automattic/simplenote-ios#1406, I identified the culprit and wrote a fix for the specific issue.

There's more work to be done in this are though. For a start, an_metadata_update_helper.rb and metadata_update_helper.rb define the same types. E.g.:

release-toolkit/lib/fastlane/plugin/wpmreleasetoolkit/helper/an_metadata_update_helper.rb

Lines 1 to 4 in 07b3152

    
           module Fastlane 
        
             module Helper 
        
               # Basic line handler 
        
               class MetadataBlock

release-toolkit/lib/fastlane/plugin/wpmreleasetoolkit/helper/metadata_update_helper.rb

Lines 1 to 4 in 07b3152

    
           module Fastlane 
        
             module Helper 
        
               # Basic line handler 
        
               class MetadataBlock

It took me a while to understand why my "breakpoints" in metadata_helper.rb (imported by gp_update_metadata_source.rb, used by ios_update_metadata_source, used by the lane that generates the .pot) weren't hit. The reason is that for some reason the Ruby interpreter loaded the ones defined in an_metadata_helper.rb, possibly due to the an_ prefix which would make that file load first? Although, I'm not sure why that would be the case since there's no import for that file.

To test

I added a dedicated unit test here. I ought to add more for other facets of the behavior, but I just wanted to fix the issue to begin with.

Also see steps from Automattic/simplenote-ios#1407.

Next step

I see a few options:

Merge this and ship a new version, hoping this is enough to cover all the paths that generate the issues we've encountered so far
Spend a bit more time on this PR, replicating the .strip on single line fix for all the other block types, then ship a new version
Invest the time right now to get to the bottom of the an_metadata_ vs metadata_ issues and improve the .pot generation logic, then ship a new version

I'm leaning towards option 2. My current implementation doesn't seem future proof, while option 3 sounds like a lot of work that could easily explode in scope.

Keen to hear what the rest of @wordpress-mobile/owl-team thinks, which is why I asked for your review even though this is still a draft.

github-actions · 2021-08-24T06:32:28Z

	1 Warning
⚠️	Please add an entry in the CHANGELOG.md file to describe the changes made by this PR

Generated by 🚫 Danger

github-actions · 2021-08-24T06:32:27Z

spec/an_metadata_update_helper_spec.rb

+require 'tmpdir'
+require_relative './spec_helper'
+
+describe Fastlane::Helper::StandardMetadataBlock do


🚫 Spec path should end with fastlane/helper/standard_metadata_block*_spec.rb.

Given there's a huge amount of work to rename our files and fix our directory structure in the plugin in order to follow the ruby dir structure conventions like we were supposed to do from the start, we can put this one in the .rubocop_todo.yml file for now until we have time(*) to do proper cleanup of all those

(*) I know, I know… not sure we'll even have that time… 😒

mokagio · 2021-08-24T06:32:44Z

🤔 Buildkite is failing on this PR because there's no pipeline to upload... But I don't see a PR to configure Buildkite in this repo.

Update: There's actually a WIP branch with no PR attached, this is the latest commit. 👌

That's all to say, we can safely ignore the Buildkite failure.

AliSoftware · 2021-08-24T13:59:50Z

For a start, an_metadata_update_helper.rb and metadata_update_helper.rb define the same types.

If only they were the only duplications 😓 There are also remainders of legacy code in some repos themselves (like WPAndroid) from before those actions were moved into the toolkit… We really need to remove those too at some point and clean up everything, but we need to check if the implementations of the actions in the repos (which take precedence over toolkit iinm) match the ones in the toolkit before blindly removing them first… so maybe let's keep that for later?

The reason is that for some reason the Ruby interpreter loaded the ones defined in an_metadata_helper.rb, possibly due to the an_ prefix which would make that file load first? Although, I'm not sure why that would be the case since there's no import for that file.

There is a mechanism in the plugin that imports/require every action+helper file automagially though – and I think that's the one that explains the behavior you saw.

AliSoftware

Thanks for debugging and tackling this!

I left some food for thought in the spec implementation, but not a blocker and I think the fix is worth landing ASAP even if we come back to this code later to clean up things – and especially get rid of the other, legacy and duplicate implementations of the same helper/action.

While you're add it and added a spec, I'd probably suggest to also add another test which covers the multi-line case, for parity. But once this is done (and maybe you take a stab at using StringIO) it should be good to merge 👍

AliSoftware · 2021-08-24T14:12:20Z

lib/fastlane/plugin/wpmreleasetoolkit/helper/an_metadata_update_helper.rb

@@ -50,7 +50,7 @@ def generate_block(fw)

        if line_count <= 1
          # Single line output
-          fw.puts("msgid \"#{File.open(@content_file_path, 'r').read}\"")
+          fw.puts("msgid \"#{File.open(@content_file_path, 'r').read.strip}\"")


🚶 Sidetrack'd comment: looking at the existing code on lines 49-62 I'm tempted to refactor all that 😅 (tho probably for a later PR)

I see we're using an optimized/uneager foreach method to consule the file line by line (great)… but only to consume the whole file anyway once… and then re-consume it again line-by-line in the else block.

At that point we might as well read the whole file into an array of lines just once (I doubt we'll have .txt files with more than, say, 100 lines, so memory consumption is unlikely to be an issue), and then test .count <= 1 directly (rather than using inject) and iterate on the array…

@mokagio Note that strip removes both whitespaces and CRLFs, and both from start and end of the string. I'm wondering if we shouldn't use chomp instead – which only removes trailing CR/LF but doesn't touches whitespaces, neither leading nor trailing – or rstrip – which removes trailing CR/LF and whitespaces, but not leading ones.

" - Hello. \n".strip # "- Hello."; notice the leading whitespace being removed too, in addition to trailing \n and whitespace " - Hello. \n".chomp # " - Hello. "; notice only the trailing \n got removed but we keep whitespaces, both leading and trailing " - Hello. \n".rstrip # " - Hello."; notice both trailing \n and whitespace got removed, but we keep leading whitespace

So TL;DR I think we should use rstrip both here and for the multiline case on line 60, and also adapt the test/spec to cover those cases too.

Good point. Better to be precise, just in case. Addressed in f7ecdfd.

AliSoftware · 2021-08-24T14:14:39Z

spec/an_metadata_update_helper_spec.rb

+require 'tmpdir'
+require_relative './spec_helper'
+
+describe Fastlane::Helper::StandardMetadataBlock do


Given there's a huge amount of work to rename our files and fix our directory structure in the plugin in order to follow the ruby dir structure conventions like we were supposed to do from the start, we can put this one in the .rubocop_todo.yml file for now until we have time(*) to do proper cleanup of all those

(*) I know, I know… not sure we'll even have that time… 😒

AliSoftware · 2021-08-24T14:20:51Z

spec/an_metadata_update_helper_spec.rb

+      output_path = File.join(dir, 'output')
+      File.open(output_path, 'w') do |file|
+        described_class.new('any-key', input_path).generate_block(file)
+      end


💄 nit: I think we couldn't use a StringIO instead here to avoid having to write to disk there.
(StringIO is a ruby built-in class that is quite useful to mock IO operations, as it's a String that acts like and has the same methods as an IO (e.g. a File), but do it all in-memory and not on-disk. We should try to use it more often in our specs imho)

I think something like this should work then (not tested):

Suggested change

output_path = File.join(dir, 'output')

File.open(output_path, 'w') do |file|

described_class.new('any-key', input_path).generate_block(file)

end

output_io = StringIO.new

described_class.new('any-key', input_path).generate_block(output_io)

Then use expect(output_io.string).to eq … below.

:TIL: ✨

That's neat, thanks. Addressed in 28dba6d.

Note that we're working on a fix for this: wordpress-mobile/release-toolkit#297

We are working on a proper fix here: wordpress-mobile/release-toolkit#297

@AliSoftware

Hat tip @AliSoftware. See #297 (comment)

@AliSoftware

Thank you for the suggestion @AliSoftware. See #297 (comment)

codecov-commenter · 2021-09-06T03:48:09Z

Codecov Report

Merging #297 (cb94546) into develop (908ff74) will increase coverage by 1.64%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #297      +/-   ##
===========================================
+ Coverage    52.03%   53.68%   +1.64%     
===========================================
  Files          108      109       +1     
  Lines         4545     4681     +136     
===========================================
+ Hits          2365     2513     +148     
+ Misses        2180     2168      -12

Impacted Files	Coverage Δ
...releasetoolkit/helper/an_metadata_update_helper.rb	`51.25% <100.00%> (+15.00%)`	⬆️
spec/android_localize_helper_spec.rb	`100.00% <0.00%> (ø)`

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 908ff74...cb94546. Read the comment docs.

As suggested here, #297 (comment), to avoid Danger warnings that we are not in a position to address right away.

mokagio · 2021-09-06T03:55:49Z

spec/an_metadata_update_helper_spec.rb

+    end
+  end
+
+  it 'does not strip a trailing new line when generating the block for a multi-line input' do


Notice "does not strip..." and the trailing \n in the last line. This is the current behavior (e.g.: here) and, unlike the one this PR aims to fix in the single-line instance, it doesn't generate a validation error.

Personally, I think we should strip that final \n, but I don't want to do it in the context of this PR and without discussing it.

Yeah it might be fine to remove the trailing \n at the end of the last line but I'm not sure either of the consequences there indeed, let's keep it for now as it doesn't do any harm.

mokagio · 2021-09-06T03:57:29Z

spec/an_metadata_update_helper_spec.rb

+      expect(output_io.string).to eq %q(msgctxt "any-key"
+msgid ""
+"Multi-line\n"
+"message\n"
+"with\n"
+"trailing new line\n"
+msgstr ""
+
+)


I would have liked to use the heredoc syntax like in the other test to avoid the difference in indentation, but that syntax escapes characters, and the comparison against the expected \n fails.

FYI, since Ruby 2.3 you can use <<~'EXPECTED' – with single-quotes around the heredoc identifier – to prevent interpolation of the content. (and the ~ is to allow indentation of the content to be ignored.

Suggested change

expect(output_io.string).to eq %q(msgctxt "any-key"

msgid ""

"Multi-line\n"

"message\n"

"with\n"

"trailing new line\n"

msgstr ""

)

expect(output_io.string).to eq <<~'EXPECTED'

msgctxt "any-key"

msgid ""

"Multi-line\n"

"message\n"

"with\n"

"trailing new line\n"

msgstr ""

EXPECTED

See https://rubyreferences.github.io/rubyref/language/literals.html#here-documents-heredocs

This is just a nitpick so not a blocker for this to be merged, but figured you'd be curious to know 😉

Oh YES! I got close to that syntax in my research for a solution yesterday, but not quite there.

mokagio · 2021-09-06T04:01:41Z

@AliSoftware I think I addressed all your comments and suggestions. Let me know what you think.

I got distracted by my support rotation last week, but it would be great to ship a new version with this before the iOS apps need to update their .po[t] when the new release notes are available. Thanks 🙇‍♂️

cc @oguzkocer if you want to point to this branch during your Simplenote iOS code freeze to test / avoid the manual fix of the .pot file.

oguzkocer · 2021-09-06T04:38:37Z

cc @oguzkocer if you want to point to this branch during your Simplenote iOS code freeze to test / avoid the manual fix of the .pot file.

I think I'll just do it manually once more (hopefully I won't forget it).

AliSoftware

Apart from the heredoc tip/suggestion, which isn't a blocker, LGTM!

@AliSoftware

Hat tip @AliSoftware. See: - #297 (comment) - https://rubyreferences.github.io/rubyref/language/literals.html#here-documents-heredocs

This won't be necessary once we ship a new release toolkit version that includes the fix from wordpress-mobile/release-toolkit#297

Still waiting for a new release toolkit PR to include the fix for this, wordpress-mobile/release-toolkit#297

github-actions bot reviewed Aug 24, 2021

View reviewed changes

This was referenced Aug 24, 2021

Test PR for release toolkit .pot fix Automattic/simplenote-ios#1407

Closed

Merge 4.43 code freeze to develop Automattic/simplenote-ios#1406

Merged

mokagio requested a review from a team August 24, 2021 06:43

AliSoftware reviewed Aug 24, 2021

View reviewed changes

mokagio added a commit to wordpress-mobile/WordPress-iOS that referenced this pull request Aug 25, 2021

Manually fix syntax error in generated AppStoreStrings.po files

69a97ef

Note that we're working on a fix for this: wordpress-mobile/release-toolkit#297

mokagio added a commit to Automattic/simplenote-ios that referenced this pull request Aug 25, 2021

Manually fix incorrect new lines in .pot file

57f7bb1

We are working on a proper fix here: wordpress-mobile/release-toolkit#297

mokagio added 3 commits September 6, 2021 13:25

Add failing test to reproduce the new line bug in the generated .pot

e74a71b

Strip trailing new line when generating .pot for "standard" block

051b0b1

Use StringIO to write POT in spec to bypass the filesystem

28dba6d

Hat tip @AliSoftware. See #297 (comment)

mokagio force-pushed the fix-msgid-newline branch from c5708ba to 28dba6d Compare September 6, 2021 03:32

Replace a strip call with a more appropriate rstrip

f7ecdfd

Thank you for the suggestion @AliSoftware. See #297 (comment)

mokagio added a commit that referenced this pull request Sep 6, 2021

Add spec/an_metadata_update_helper_spec.rb to .rubocop_todo.yml

0554ff0

As suggested here, #297 (comment), to avoid Danger warnings that we are not in a position to address right away.

mokagio marked this pull request as ready for review September 6, 2021 03:53

mokagio added 2 commits September 6, 2021 13:58

Add test for multi-line string POT block generation

27a1c74

Add spec/an_metadata_update_helper_spec.rb to .rubocop_todo.yml

b3b1628

As suggested here, #297 (comment), to avoid Danger warnings that we are not in a position to address right away.

mokagio force-pushed the fix-msgid-newline branch from 0554ff0 to b3b1628 Compare September 6, 2021 03:58

mokagio commented Sep 6, 2021

View reviewed changes

mokagio requested a review from AliSoftware September 6, 2021 03:58

AliSoftware approved these changes Sep 6, 2021

View reviewed changes

Replace a %q() with squiggly heredoc with disabled interpolation

cb94546

Hat tip @AliSoftware. See: - #297 (comment) - https://rubyreferences.github.io/rubyref/language/literals.html#here-documents-heredocs

mokagio changed the title ~~Add failing test to reproduce the new line bug in the generated .po~~ Strip trailing new lines in single line msgid when generating .po[t] file Sep 7, 2021

mokagio merged commit 603e6f6 into develop Sep 7, 2021

mokagio deleted the fix-msgid-newline branch September 7, 2021 01:53

mokagio added a commit to wordpress-mobile/WordPress-iOS that referenced this pull request Sep 22, 2021

Fix incorrect msgid in generated .po files

7b1dadf

Still waiting for a new release toolkit PR to include the fix for this, wordpress-mobile/release-toolkit#297

This was referenced Oct 5, 2021

Merge 4.46 code freeze to develop Automattic/simplenote-ios#1458

Merged

Adds a changelog for "strip trailing newline" bug fix #301

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Strip trailing new lines in single line `msgid` when generating `.po[t]` file #297

Strip trailing new lines in single line `msgid` when generating `.po[t]` file #297

mokagio commented Aug 24, 2021 •

edited

Loading

github-actions bot commented Aug 24, 2021

github-actions bot Aug 24, 2021 •

edited

Loading

AliSoftware Aug 24, 2021

mokagio commented Aug 24, 2021 •

edited

Loading

AliSoftware commented Aug 24, 2021 •

edited

Loading

AliSoftware left a comment

AliSoftware Aug 24, 2021

AliSoftware Aug 25, 2021

mokagio Sep 6, 2021

AliSoftware Aug 24, 2021

AliSoftware Aug 24, 2021

mokagio Sep 6, 2021

codecov-commenter commented Sep 6, 2021 •

edited

Loading

mokagio Sep 6, 2021

AliSoftware Sep 6, 2021

mokagio Sep 6, 2021

AliSoftware Sep 6, 2021 •

edited

Loading

mokagio Sep 7, 2021

mokagio commented Sep 6, 2021

oguzkocer commented Sep 6, 2021

AliSoftware left a comment

	module Fastlane
	module Helper
	# Basic line handler
	class MetadataBlock

Strip trailing new lines in single line msgid when generating .po[t] file #297

Strip trailing new lines in single line msgid when generating .po[t] file #297

Conversation

mokagio commented Aug 24, 2021 • edited Loading

To test

Next step

github-actions bot commented Aug 24, 2021

github-actions bot Aug 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mokagio commented Aug 24, 2021 • edited Loading

AliSoftware commented Aug 24, 2021 • edited Loading

AliSoftware left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 6, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AliSoftware Sep 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mokagio commented Sep 6, 2021

oguzkocer commented Sep 6, 2021

AliSoftware left a comment

Choose a reason for hiding this comment

Strip trailing new lines in single line `msgid` when generating `.po[t]` file #297

Strip trailing new lines in single line `msgid` when generating `.po[t]` file #297

mokagio commented Aug 24, 2021 •

edited

Loading

github-actions bot Aug 24, 2021 •

edited

Loading

mokagio commented Aug 24, 2021 •

edited

Loading

AliSoftware commented Aug 24, 2021 •

edited

Loading

codecov-commenter commented Sep 6, 2021 •

edited

Loading

AliSoftware Sep 6, 2021 •

edited

Loading