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 localization bundle from VTAcknowledgementsViewController and remove CocoaPods project integration #11

Merged
merged 11 commits into from
Mar 31, 2016

Conversation

gerbiljames
Copy link
Contributor

This PR is a bit more complex...

I had to run 'pod update' with cocoapods 0.39.0 to add the bundle to the project which changed a lot of files. The tests pass on my machine but I wouldnt merge this until the Travis issue is sorted.

Also, thanks for accepting my other PRs so quickly.

@vtourraine
Copy link
Owner

That’s awesome. Totally makes sense to merge the localization resources from VTAcknowledgementsViewController.

It’s nice that you’ve been able to fix the Travis problem. It’s related to their install of xctool, apparently, so I’m sure that it will be fixed very soon. Anyway, that bypasses the problem for now, we’ll just need to revert it to the original Travis config later on (cleaner than this custom build script).

My only problem with this code is that it’d be great to have it unit-tested. Unfortunately, there’s no trivial way to mock the global NSLocale values. Maybe that means that I should refactor that code.

Anyway, thanks again for the PR, I’ll merge it in a couple of days.

@gerbiljames
Copy link
Contributor Author

Thanks, I've had a go at some unit tests in the localization-tests branch of my fork of this repo. Theres two issues with them though. I had to mark the localization functions in AcknowListViewController as public so they could be called from the test target (@testable import would fix this but since the AcknowList target is managed by CocoaPods it can't be enabled for testability easily).

The other issue is that in order to mock AcknowListViewController init(coder:) must be a designated initializer or you get a compiler error:

<unknown>:0: error: must call a designated initializer of the superclass 'AcknowListViewController' AcknowList.AcknowListViewController:26:33: note: convenience initializer is declared here required public convenience init(coder aDecoder: NSCoder)

and in order to make it a designated initializer I had to shift all of the implementation into it and call super.init(style:) from it. It adds a little duplicate code but I couldn't find any other way to fix the error.

@vtourraine
Copy link
Owner

Again, that looks great. You should just merge that “tests” branch to your “localization” branch, to include it in this PR.

I have no experience with mocking in Swift, I though it’d be more convoluted, but that local subclassing/overriding seems like a nice solution.

About exposing the private method: I’d rather drop the CocoaPods tests workspace configuration and use @testable, instead of making unnecessary methods public. But there might be another solution, by explicitly adding the source file to the test target (source). Could you try this out?

About the “init with coder” as a designated initializer, feel free to update it. The view controller life cycle doesn’t play nice with Swift non-optionals and designated initializers, so if we need to change that, so be it.

@gerbiljames
Copy link
Contributor Author

I tried adding the source files to the test target but this caused issues with the bundle not being accessible from there. Instead, I enabled the Enable Testability flag on the AcknowList target and just used @testable import. I'm not sure if CocoaPods will overwrite this on running pod update but its easily fixed by reenabling the flag if it does.

I do think moving away from the CocoaPods workspace config is a good idea anyway as it gives greater control over the workspace and it isn't being used to manage dependencies since there aren't any.

@vtourraine
Copy link
Owner

So not as easy as I would have hoped, but that’s interesting. Well, you can get rid of the CocoaPods configured workspace then. There’s the pod deintegrate command, and we only need to keep the xcodeproj. And we’ll have to edit the Travis configuration again, to drop the workspace.

Feel free to add these changes to this PR (it’s already too big, we might as well add that too). If you don’t have time for this, I can also merge this PR as it is, and we’ll remove the CocoaPods configured workspace later.

@gerbiljames
Copy link
Contributor Author

I was worried myself about this PR getting too big, but I guess we're down the rabbit hole now. I'll remove the CP workspace later today.

@gerbiljames
Copy link
Contributor Author

Okay, CocoaPods has been removed from the project. I've reorganised the workspace structure so the test target is attached to the AcknowList target instead of the example project. Both the example and framework projects are referenced in the workspace in the root.

pod lib lint ran successfully and I've tested the library using the CP :path option in one of my own apps.

@gerbiljames gerbiljames changed the title Add localization bundle from VTAcknowledgementsViewController Add localization bundle from VTAcknowledgementsViewController and remove CocoaPods project integration Mar 30, 2016
@vtourraine
Copy link
Owner

Awesome. Thank you so much for your contribution.

I’ll just wait a bit before tagging it as a new release, to let people try it out.

@vtourraine vtourraine merged commit b02cdda into vtourraine:master Mar 31, 2016
@gerbiljames gerbiljames deleted the add-localization-bundle branch April 3, 2016 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants