Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix multiple code paths #267

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix multiple code paths #267

wants to merge 2 commits into from

Conversation

mman
Copy link
Contributor

@mman mman commented Oct 11, 2022

Fixes #264

Proposed Changes

I have adjusted the logic of incrementalCodeUpdate method to properly handle extracting strings from source files specified via multiple entries in update.code.codePaths option.

In the past, the logic was to walk all codePaths directories. For each directory create a temporary .strings file. Merge all entries from the temporary .strings file into output. Invoking this logic for multiple codePaths in additive=false mode would repeatedly overwrite the output Localized.strings file with only the entries extracted from currently processed directory and would remove everything else.

The new logic uses only one temporary .strings file into which all entries from all codePaths are extracted by using extractLocStrings -a (the append mode) and only after that are all strings from the temporary .strings file merged into the output Localizable.strings.

Changes

  • When invoking extractLocStrings specify the -a option to append extracted strings into temporary .strings file instead of replacing them.
  • Use just one temporary .strings file for all strings extracted during code.update

Testing

I have a testing project with the following .bartycrouch.toml
Multiple entries via update.code.codePaths, and update.code.additive=false
Each subdirectory contains one NSLocalizedString entry.

[update]
tasks = ["code", "normalize"]

[update.code]
codePaths = ["Sources/TestBartyCrouch", "Sources/TestBartyCrouch1"]
localizablePaths = ["."]
additive = false

[update.normalize]
paths = ["."]
sourceLocale = "en"

[lint]
paths = ["."]
subpathsToIgnore = [".git", "carthage", "pods", "build", ".build", "docs"]
duplicateKeys = true
emptyValues = true

Invoking published release against this project results in only one entry detected:

€ rm en.lproj/Localizable.strings && touch en.lproj/Localizable.strings

€ bartycrouch update -v                                                
Starting Task 'Update Code' ...
2022-10-11 16:37:39.523: ℹ️  Incrementally updated keys of file '/Users/mman/Developer/TestBartyCrouch/en.lproj/Localizable.strings'.
2022-10-11 16:37:39.527: ✅  Sources/TestBartyCrouch:  Successfully updated strings file(s) of Code files.
2022-10-11 16:37:39.596: ℹ️  Incrementally updated keys of file '/Users/mman/Developer/TestBartyCrouch/en.lproj/Localizable.strings'.
2022-10-11 16:37:39.597: ✅  Sources/TestBartyCrouch1:  Successfully updated strings file(s) of Code files.
Task 'Update Code' took 0.144 seconds.
Starting Task 'Normalize' ...

€ cat en.lproj/Localizable.strings 
/* Greeting from TestBartyCrouch1 target */
"Hello, World, 1!" = "";

Invoking version with the new logic results in:

€ rm en.lproj/Localizable.strings && touch en.lproj/Localizable.strings
€ ../BartyCrouch/.build/debug/bartycrouch update -v
Starting Task 'Update Code' ...
2022-10-11 16:38:53.051: ✅  Sources/TestBartyCrouch:  Successfully updated strings file(s) of Code files.
2022-10-11 16:38:53.120: ✅  Sources/TestBartyCrouch1:  Successfully updated strings file(s) of Code files.
2022-10-11 16:38:53.123: ℹ️  Incrementally updated keys of file '/Users/mman/Developer/TestBartyCrouch/en.lproj/Localizable.strings'.
Task 'Update Code' took 0.141 seconds.
Starting Task 'Normalize' ...

€ cat en.lproj/Localizable.strings 
/* Greeting from TestBartyCrouch target */
"Hello, World!" = "";

/* Greeting from TestBartyCrouch1 target */
"Hello, World, 1!" = "";

@mman mman mentioned this pull request Oct 11, 2022
Open
@Jeehut
Copy link
Member

Jeehut commented Oct 13, 2022 •
edited
Loading

@mman Thank you for this. Just want to let you know that this is on my radar and I quickly skimmed through the changes and noticed that this needs a bit more detailed check, there are some things you've changed here which require a more detailed look to not break things. I don't have enough time right now to do this, but I'll get back to it as soon as possible.

Feel free to ping me if you don't hear from me after 10 days.

@Jeehut Jeehut self-requested a review October 13, 2022 21:25
@hactar
Copy link
Contributor

hactar commented Apr 5, 2024

@Jeehut We have just run into the same issue documented here #264 - if this PR fixes this, any chance we could have it merged please? ☺️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jeehut Jeehut Awaiting requested review from Jeehut

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug] update.code with additive=false and code in multiple parent directories looses strings
3 participants
@mman @Jeehut @hactar