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 fallback translation bundle method #5314

Merged
merged 4 commits into from
Jan 6, 2023

Conversation

jhugman
Copy link
Contributor

@jhugman jhugman commented Jan 5, 2023

Related to EXP-3042.

The functional part of this is the adding a fallback translation bundle lookup method.

It's actually quite difficult to test, given that there are no strings in the ios megazord, so there exists a duplicate method in Firefox for iOS. We include it here since we want to use it in Focus.

Because Nimbus doesn't know the internal structure of the bundle it can only provide this fallbackTranslationBundle() method— it will be up to the app to provide the list of bundles themselves.

The old way of doing this was to call Nimbus.create(… resourceBundles: [Bundle.main, Strings.bundle] ). The new way of doing this will be:

let bundles = [
   Bundle.main,
   Strings.bundle,
   Strings.bundle.fallbackTranslationBundle(),
].compactMap { $0 }

NimbusBuilder()
    .with(resourceBundles: bundles)
    
    .build()

Pull Request checklist

  • Quality: This PR builds and tests run cleanly
    • Note:
      • For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
      • If this pull request includes a breaking change, consider cutting a new release after merging.
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry in CHANGES_UNRELEASED.md or an explanation of why it does not need one
    • Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
  • Dependencies: This PR follows our dependency management guidelines
    • Any new dependencies are accompanied by a summary of the due dilligence applied in selecting them.

Branch builds: add [ff-android: firefox-android-branch-name] and/or [fenix: fenix-branch-name] to the PR title.

@jhugman jhugman force-pushed the jhugman/exp-3042-ios-add-fallback-translation-bundle branch from 46ea619 to 8267577 Compare January 5, 2023 16:59
@codecov-commenter
Copy link

Codecov Report

Base: 46.57% // Head: 46.56% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (46ea619) compared to base (b8ba517).
Patch has no changes to coverable lines.

❗ Current head 46ea619 differs from pull request most recent head 8267577. Consider uploading reports for the commit 8267577 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5314      +/-   ##
==========================================
- Coverage   46.57%   46.56%   -0.02%     
==========================================
  Files         180      180              
  Lines       14449    14450       +1     
==========================================
- Hits         6729     6728       -1     
- Misses       7720     7722       +2     
Impacted Files Coverage Δ
.../support/rc_crypto/nss/nss_build_common/src/lib.rs 66.33% <0.00%> (-0.67%) ⬇️
components/nimbus/src/behavior.rs 89.83% <0.00%> (-0.41%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@jhugman jhugman requested a review from jeddai January 5, 2023 17:45
Copy link
Member

@jeddai jeddai left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@@ -30,7 +30,7 @@ public class NimbusBuilder {
* A closure for reporting errors from Rust.
*/
@discardableResult
func withErrorReporter(_ reporter: @escaping NimbusErrorReporter) -> NimbusBuilder {
func with(errorReporter reporter: @escaping NimbusErrorReporter) -> NimbusBuilder {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not familiar with this code format, could you briefly explain what's happening here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Swift has named arguments as part of the name of the method, and the Swift trend has been to embrace that, so now we write:

builder.with(errorReporter: theReporter)

instead of:

builder.withErrorReporter(theReporter)

The syntax above is:

func methodName(namedArg localName: Type)

where namedArg is _, then it's omitted when calling (like withErrorReporter(theReporter)).
and localName is optional, and that's the name of the argument in the method.

func methodName(namedArg localName: Type) {

}

is syntactic sugar for:

func methodName(namedArg: Type) {
   let localName = namedArg

}

Let me know if this does/doesn't help.

@jhugman jhugman merged commit 4c2aed4 into main Jan 6, 2023
@jhugman jhugman deleted the jhugman/exp-3042-ios-add-fallback-translation-bundle branch January 6, 2023 13:54
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