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.

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

Add option to identify the source library when merging strings.xml files #351

Merged
merged 5 commits into from
Apr 11, 2022

Conversation

AliSoftware
Copy link
Contributor

@AliSoftware AliSoftware commented Apr 7, 2022

This improves the existing an_localize_libs action to adds support for the source_id option.

This option aims to helps identify which library each <string> merged into the main strings.xml came from.

Why

  • This is prep work towards the plan drafted in paaHJt-3fg-p2
    • The future goal here would be to later update the merge logic to delete all the old entries that came from a previous merge of a library, before merging the new strings again. That way, keys deleted from the lib's strings.xml in a later version would properly be removed from the app's strings.xml, instead of being kept in the apps' strings.xml forever while not being used nor useful anymore
    • I decided to keep that deletion feature for later though, because implementing that feature would likely require a larger change in the current implementation and logic of the an_localize_libs's helper code, and also might require a deeper analysis of potential consequences before going that route (e.g. what if a string is deleted from a lib later… but that key was still used by the app itself too?). Still, adding an attribute to identify the libs the strings were coming from still felt a useful first step anyway
  • But also, it will just help us (both the developers and the platform team) understand where each string from the strings.xml is coming from
    • This will be useful to developer to understand, when a string they see in the WordPress/src/main/res/strings.xml, where the source of truth for that string actually comes from (the app, or a lib?) and thus where to change that value if they need to (no point in changing the copy in the app's strings.xml file if the key will be re-overwritten by the next lib merge!)
    • It will also allow them to better reason about when to use content_override if they do want the key from the app's strings.xml to always take over and not be replace by the same key from a lib in the future (useful for example from a key like app_name not to be overwritten by a merge of the about library if it were to define one as well…)

How

The action now accepts a new key in the Hash passed to its libs_strings_path parameter.
That new key is named source_id and allows the caller to provide an (optional) "identifier" string for the library. If provided, that identifier will be added as the value of a custom a8c-src-lib attribute of the <string …> XML nodes added or updated during the merging of the strings of said library.

You can see the result in action in this draft WPAndroid PR.

This PR also takes the occasion to:

  • Fix a crash if the caller provided a nil value for the exclusions key in the Hash
  • Use UI.message and UI.verbose instead of puts when the helper printed information about the strings being merged or updated.

…rings from libs

To help identify them more easily in the merged file

and make it clear for developers where they come from (and thus where the source of truth for each string is)
So that both omitting the key entirely, and providing it but with a nil value, are both the same as using an empty exclusion list
@AliSoftware AliSoftware self-assigned this Apr 7, 2022
Copy link
Contributor

@mokagio mokagio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested this via wordpress-mobile/WordPress-Android#16280. Worked as described. Nothing to comment 👍

CHANGELOG.md Outdated Show resolved Hide resolved
Comment on lines -16 to +18
puts " - Skipping #{string_line.attr('name')} string"
UI.message " - Skipping #{string_line.attr('name')} string"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor fix, but always appreciated 👨‍🍳 👌

Co-authored-by: Gio Lodi <giovanni.lodi42@gmail.com>
@AliSoftware AliSoftware enabled auto-merge April 11, 2022 18:12
@AliSoftware AliSoftware merged commit 5cf371e into trunk Apr 11, 2022
@AliSoftware AliSoftware deleted the libs-merge/source-id branch April 11, 2022 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants