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

Introduce Locale helper models #296

Draft
wants to merge 10 commits into
base: trunk
Choose a base branch
from
Draft

Introduce Locale helper models #296

wants to merge 10 commits into from

Conversation

AliSoftware
Copy link
Contributor

@AliSoftware AliSoftware commented Aug 10, 2021

This PR introduces the Locales model class in the release toolkit. Its intent is to work way more easily with locale codes in our Fastfiles, by replacing huge constant lists like those 65 lines in WPAndroid's Fastfile with way shorter and nicer lines like:

RELEASE_NOTES_LOCALES  = Fastlane::Locales.mag16

This will return an Array<Locale> where Locale is a struct with 5 fields containing the locale codes for each space (glotpress, android, google_play, ios, app_store).

A Locale can also be converted into a Hash for retro-compatibility with the existing toolkit actions that expect such a Hash (even though in the future we might want to support both a Hash and a Locale for those actions, but that's for a separate story)
This means we will be able to replace those WCAndroid 18 lines with just this – using .map(&:to_h) to convert it from an Array<Locale> to an Array<Hash> for compatibility with existing actions:

SUPPORTED_LOCALES = Fastlane::Locales[%w[ar de es fr he id it ja ko nl pt-br ru sv tr zh-cn zh-tw]].map(&:to_h)

Draft State

We still need to

  • Complete the list of ALL_KNOWN_LOCALES: add the missing values for ios and app_store keys, then ensure all the codes are correct for all the various keys, and that we are not missing any known locale.
  • Enable from_ios and from_app_store family of tests once we have the values in ALL_KNOWN_LOCALES

@mokagio I've added you as preliminary reviewer, feel free to take a first look (especially at the Locales class API and at the tests) and provide initial feedback. I'll add the whole Owl team as final reviewers once ☝️ is completed and I undraft the PR.

@AliSoftware AliSoftware self-assigned this Aug 10, 2021
spec/locales_spec.rb Outdated Show resolved Hide resolved
spec/locales_spec.rb Outdated Show resolved Hide resolved
spec/locales_spec.rb Outdated Show resolved Hide resolved
spec/locales_spec.rb Outdated Show resolved Hide resolved
spec/locales_spec.rb Outdated Show resolved Hide resolved
spec/locales_spec.rb Outdated Show resolved Hide resolved
spec/locales_spec.rb Outdated Show resolved Hide resolved
spec/locales_spec.rb Outdated Show resolved Hide resolved
spec/locales_spec.rb Outdated Show resolved Hide resolved
spec/locales_spec.rb Outdated Show resolved Hide resolved
spec/locales_spec.rb Outdated Show resolved Hide resolved
spec/locales_spec.rb Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Aug 10, 2021

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

Generated by 🚫 Danger

@codecov-commenter
Copy link

codecov-commenter commented Aug 10, 2021

Codecov Report

Merging #296 (61147cf) into develop (e02e57d) will increase coverage by 1.08%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #296      +/-   ##
===========================================
+ Coverage    51.64%   52.72%   +1.08%     
===========================================
  Files          108      111       +3     
  Lines         4533     4637     +104     
===========================================
+ Hits          2341     2445     +104     
  Misses        2192     2192              
Impacted Files Coverage Δ
lib/fastlane/plugin/wpmreleasetoolkit.rb 100.00% <100.00%> (ø)
...astlane/plugin/wpmreleasetoolkit/models/locales.rb 100.00% <100.00%> (ø)
spec/locale_spec.rb 100.00% <100.00%> (ø)
spec/locales_spec.rb 100.00% <100.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 e02e57d...61147cf. Read the comment docs.

class Locales
###################
## Constants
ALL_KNOWN_LOCALES = [
Copy link
Contributor Author

@AliSoftware AliSoftware Aug 10, 2021

Choose a reason for hiding this comment

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

WIP: To be completed with ios: and app_store: values for each locale; we'll also need to double-check all the tuples to ensure we defined the correct ones.

spec/locale_spec.rb Outdated Show resolved Hide resolved
spec/locales_spec.rb Outdated Show resolved Hide resolved
spec/locales_spec.rb Outdated Show resolved Hide resolved
spec/locales_spec.rb Outdated Show resolved Hide resolved
spec/locales_spec.rb Outdated Show resolved Hide resolved
spec/locales_spec.rb Outdated Show resolved Hide resolved
spec/locales_spec.rb Outdated Show resolved Hide resolved
spec/locales_spec.rb Outdated Show resolved Hide resolved
spec/locales_spec.rb Outdated Show resolved Hide resolved
spec/locales_spec.rb Outdated Show resolved Hide resolved
spec/locales_spec.rb Outdated Show resolved Hide resolved
@AliSoftware
Copy link
Contributor Author

AliSoftware commented Aug 11, 2021

I'm actually unsure if we use/need list of locale codes in the iOS Fastfiles like we do in the Android ones 🤔 at least yet?

A quick look seem to indicate that we don't really rely on any hardcoded list for iOS, since when post-processing locales we just iterate over folder names in fastlane/metadata, and the only other place where we need the list of locales is when we download them from GlotPress, for which, as of today, we still do it via the download_metadata.swift Swift script, not via a ruby action where we could use that new Fastlane::Locales[…] magic.

But I still think it would be nice to have those ios/app store locale codes in that list even if we don't yet really use them in our Fastfile, so that when we get the opportunity work on the "Improve Localization tooling" project (paaHJt-sd-p2) and likely migrate that Swift script into a fastlane action by then, we'd have those locale codes ready to be used.


Another point: unlike android's values-<code> vs PlayStore locale codes being different, for iOS it seems (at least at first, from the couple ones I quickly checked) that the locale codes used in iOS (<code>.lproj) and in AppStore are the same (some consistency at least, not like Android 😅 ), which means I'm unsure if there's a point keeping both :ios and :app_store keys in the Locale struct (allows to get a nice mirroring with Android) vs only have the :ios key (would break the symmetry, but avoids repetition). A middle ground would be to only use :ios in the initializer and list of Struct fields, and declare alias_method :app_store, :ios as an alias for ios.

@mokagio Thoughts?

spec/locales_spec.rb Outdated Show resolved Hide resolved
lib/fastlane/plugin/wpmreleasetoolkit/models/locales.rb Outdated Show resolved Hide resolved
@mokagio mokagio closed this Aug 23, 2021
@mokagio mokagio reopened this Aug 23, 2021
@mokagio
Copy link
Contributor

mokagio commented Aug 23, 2021

Sorry about closing the PR I went to click the text area but hit the close button 😮‍💨

@mokagio
Copy link
Contributor

mokagio commented Aug 23, 2021

Sorry for taking so long to reply!

But I still think it would be nice to have those ios/app store locale codes in that list even if we don't yet really use them in our Fastfile

+1 there's value in having them there for sure.

A middle ground would be to only use :ios in the initializer and list of Struct fields, and declare alias_method :app_store, :ios as an alias for ios.

I like this idea very much.

Still requires:
 - Completing the list of ALL_KNOWN_LOCALES
 - Enabling from_ios and from_app_store family of tests once we have the values
…ting point + enable missing corresponding specs
When run with a modern git version which supports `--initial-branch`, git init was run twice due to using `|` instead of expected `||` in the shell commands
…sed for each key/context

+ Split locale.rb and locales.rb
Per ruby conventions, as they might raise on invalid/unknown values
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.

3 participants