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

MVP for Bazel Support #6

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

maxwellE
Copy link

@maxwellE maxwellE commented Nov 13, 2023

Adds a new bazel rule localize that allows Bazel consumers to use this off the shelf in their builds.

To test simply run: bazel test //Example/... --apple_platform_type=ios

Allows small strings to be consumed
by bazel directly
@maxwellE maxwellE force-pushed the maxwelle/bazel-support branch from f2ab098 to c7edd98 Compare November 13, 2023 16:03
});
// Haven't tested with CFBundleAllowMixedLocalizations set to YES, although it seems like that'd be handled by the
// NSLocalizedString fallback
return sKeyToString[key] ?: NSLocalizedString(key, @"");
}

NSString *SSTStringForKeyWithBundleAndSubdirectory(NSString *key, NSBundle *bundle, NSString *subdirectory)
Copy link
Member

Choose a reason for hiding this comment

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

We intentionally avoid having the NSBundle as part of the public function because SmallStrings should always be used with just one localization file per language, not spread out into multiple bundles. That's because you don't really get compression benefits if the data is split up into multiple files before being compressed. Also, because of the minimum files size: https://docs.emergetools.com/docs/avoid-many-files you can actually end up increasing app size by splitting the translation keys out into a separate file (like SmallStrings does) if you are spreading localizations across many files.

Copy link
Author

Choose a reason for hiding this comment

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

Will refactor with this approach in mind

Copy link
Member

@noahsmartin noahsmartin left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @maxwellE ! I left one comment about the small strings code that might cause this approach to not see very significant size savings (or even a size increase). None of the maintainers of this repo have experience working with Bazel, so I we think it would make sense to keep this as a separate branch that people can reference. We aren't planning on adding anything to the core functionality, so it should stay up to date.

Also, it might be worth taking a look at this talk from Airbnb about how they use the approach in SmallStrings to handle their localizations: https://www.youtube.com/watch?v=UKqPqtvZtck I don't think they needed the additional Bazel rules so maybe there is a simpler way to do it?


@end

int main(int argc, char *argv[]) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this can use the existing localize.sh script?

Copy link
Author

Choose a reason for hiding this comment

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

The bazel logic handles everything you have in that shell in a hermetic way and also sidesteps the need for ruby to run this (which is likely not needed since its essentially doing globbing)

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