Skip to content
This repository has been archived by the owner on Jul 8, 2019. It is now read-only.

Bundle Resources #219

Closed

Conversation

Brok3nHalo
Copy link
Contributor

Fixed issue where pod installed the supported languages into main bundle causing applications to report possibly unsupported languages.

…dle causing applications to report possibly unsupported languages.
@coveralls
Copy link

coveralls commented May 12, 2016

Coverage Status

Coverage increased (+17.7%) to 38.369% when pulling 3e40ccf on Brok3nHalo:bugfix/bundle_resources into 6233e7c on mattt:master.

@yas375
Copy link
Member

yas375 commented May 12, 2016

@Brok3nHalo do you have use_frameworks! in your Podfile? Or do you integrate as a static library?

@Brok3nHalo
Copy link
Contributor Author

@yas375 static libraries, I actually didn't know about use_frameworks! but we unfortunately are using some pods that aren't compatible with it right now and it looks like an all or nothing deal.

Researching into the language issue, I found the official recommendation is to use resource_bundles instead of resources unless you specifically need resources added to the main bundle because of this as well as potential for name collisions with other resources and optimization issues as noted at http://guides.cocoapods.org/syntax/podspec.html#resources

@yas375
Copy link
Member

yas375 commented May 14, 2016

@Brok3nHalo what you are saying makes sense. I agree we should adopt resource_bundles.

I'm a little skeptical about having different code paths ([NSBundle formatterKitBundle]?: [NSBundle bundleForClass:[self class]]). I'd like to have one code path for all options of integrations into other projects:

  • CocoaPods framewrok
  • CocoaPods static lib
  • Carthage (framework)
  • and Swift Package Manager in the nearest future

I'm not 100% sure if this is possible, but I hope so :)

Also, to feel myself more confident when accepting changes like this I'd like to add tests for integration of the library using various methods. I'm going to do this first and then let's return to this pull request. Hopefully I'll have some time to do this over the next week or so.

@yas375
Copy link
Member

yas375 commented May 23, 2016

@Brok3nHalo I have added tests I was talking about - #221. I also have tried locally to apply your changes and run these new integration tests and everything seems to be good 👍

I'd like to clarify a couple things.

Fixed issue where pod installed the supported languages into main bundle causing applications to report possibly unsupported languages.

  1. How can I see this with a sample app? Where does it report unsupported languages?
  2. Currently in Readme there is a section about removing unused localizations. It was added before I've started to maintain the project, so I don't really know much details about it. Do you think that section can be removed with switching to resource bundles?

@Brok3nHalo
Copy link
Contributor Author

Awesome!

  1. I think the easy way to test is to create a app that uses the library as a static library pod (i.e not using use_frameworks!). Maybe add a couple languages so you have a expected set to check against. Then at runtime check [[NSBundle mainBundle] localizations]. The result should be only the languages your sample app supports but currently includes FormatterKit's languages since they're included in the main bundle.
  2. I think after this the readme will be accurate. I forgot to mention in the original PR comment that this change was based off a previous change to the podspec that got lost in a refactor and the readme actually already assumes the resources are bundled this way.

mattt@eea311a attempted the same thing but I'm guessing didn't work because it didn't modify the code to actually use the bundle.

My change only effects when used from a pod and not when it's used as a static library or linked directly so the current additional instructions for removing the extra languages still apply in those cases. With a few modifications it could always use the bundle even in those scenarios but I didn't want to mess with the project file for scenarios I wasn't using myself in case it caused any unforeseen issues with build settings.

@yas375
Copy link
Member

yas375 commented Jun 11, 2016

@Brok3nHalo I'm sorry for the delay with this on my side. I think I'll get back to this after WWDC week.

I did look into this though:
2016-06-10_1727

I'm still a little concerned about using different ways to access NSBundle depending on setup. Ideally I'd like to have same code path while the lib can be integrated with different methods. I was thinking that maybe we can create a bundle with localizations inside the lib itself instead of just having localization files. And then we can keep using resources in the podspec, but it will be ss,resources = 'FormatterKit/FormatterKit.bundle'. I suppose this way we will have same code path. Does this sound good to you?

yas375 added a commit that referenced this pull request Aug 30, 2016
yas375 added a commit that referenced this pull request Aug 30, 2016
This proves the issue described in #219.
@yas375 yas375 mentioned this pull request Sep 2, 2016
@yas375
Copy link
Member

yas375 commented Sep 2, 2016

Subsumed by #229.

@yas375 yas375 closed this Sep 2, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants