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

Adopt Overcoat to work with Mantle 2.0 #87

Merged
merged 3 commits into from Jul 8, 2015

Conversation

sodastsai
Copy link
Member

Following are changes:

  • Add OVERCOAT_USING_MANTLE_2 in OVCUtilities.h as macro used to separate codebase for different Mantle versions.
  • Separate CoreData support and Social framework support
    • In Mantle 2.0, CoreData support is not provided by default, it comes another submodule. (MTLManagedObjectAdapter)
    • Since Social.framework support is not a required module of Overcoat, it should be a submodule.
    • This is a backward incompatible change.

      To use CoreData, even if you are using Mantle 1.x, you should use pod 'Overcoat/CoreData'
      To use Social.framework, even if you are using Mantle 1.x, you should use pod 'Overcoat/Social'

  • Refine Xcode project used to run tests.
    • Integrate Overcoat with test targets by cocoapods. (Overcoat was integrated with test targets by Xcode's settings originally, but we should test for cocoapods)
    • You can use make test to run tests targeting on both iOS/OS X and Mantle 1.x/2.x
    • And since Social and CoreData have been separate as submodules, their tests are also separated.
    • Update OHHTTPStubs to 4.x
    • The Podfile attached is used to run tests. You could pass environment variables to decide what to install (for tests). For example, OS_TYPE=OSX MANTLE=2.0 pod install will setup cocoapods to install packages required for testing on OS X with Mantle 2.0

@sodastsai sodastsai mentioned this pull request Jun 16, 2015
This commit makes Overcoat work with both Mantle 1.x and Mantle 2.x
Following are changes:

- Add `OVERCOAT_USING_MANTLE_2` in `OVCUtilities.h` as macro used to separate codebase for different Mantle versions.
- Separate `CoreData` support and `Social` framework support
  * In Mantle 2.0, CoreData support is not provided by default, it comes another submodule. (`MTLManagedObjectAdapter`)
  * Since `Social.framework` support is not a required module of Overcoat, it should be a submodule.
  * This is a backward incompatible change.
    > To use CoreData, even if you are using Mantle 1.x, you should use `pod 'Overcoat/CoreData'`
    > To use Social.framework, even if you are using Mantle 1.x, you should use `pod 'Overcoat/Social'`
- Refine Xcode project used to run tests.
  * Integrate `Overcoat` with test targets by **cocoapods**. (Overcoat was integrated with test targets by Xcode's settings originally, but we should test for cocoapods)
  * You can use `make test` to run tests targeting on both iOS/OS X and Mantle 1.x/2.x
  * And since `Social` and `CoreData` have been separate as submodules, their tests are also separated.
  * Update `OHHTTPStubs` to 4.x
  * The `Podfile` attached is used to run tests. You could pass environment variables to decide what to install (for tests). For example, `OS_TYPE=OSX MANTLE=2.0 pod install` will setup cocoapods to install packages required for testing on OS X with Mantle 2.0
@sodastsai
Copy link
Member Author

@gonzalezreal any idea for this?

@ryanmaxwell
Copy link

I'm hoping this is merged soon

@sodastsai
Copy link
Member Author

@ryanmaxwell Great 👍 if you want test with it now, you could use pod 'Overcoat', :git => 'https://github.com/sodastsai/Overcoat.git', :branch => 'mantle2'

My app is using it (on the App Store) and things seems to be okay :)

@gonzalezreal
Copy link
Contributor

@sodastsai Thanks a lot for the contribution! I will look into it soon.

@lexrus
Copy link

lexrus commented Jul 2, 2015

@sodastsai Nice. Could you also upgrade PromiseKit to 2.0.6?

@sodastsai
Copy link
Member Author

@lexrus I am not familiar with and don't use PromiseKit, so I could not help you. If possible, you could also create a new PR for this.

@gonzalezreal
Copy link
Contributor

@sodastsai Thanks again for the contribution. First of all, sorry for taking so long to look into this. I added you as a collaborator so I don't stall your contributions.

I had a chance to have a look at it and everything looks fine. Did you get any feedback from other users about this PR?

@sodastsai sodastsai merged commit 703fc11 into Overcoat:master Jul 8, 2015
@sodastsai
Copy link
Member Author

@gonzalezreal I've merged back to the master branch. and so let's prepare to update the pod. if possible, could you please add me as one of authors? Then after a period to wait for more feedback, I'll update the pod, Thanks.

(Via pod trunk add-owner Overcoat sodas2002@gmail.com)

@sodastsai sodastsai deleted the mantle2 branch July 8, 2015 15:05
@sodastsai sodastsai restored the mantle2 branch July 8, 2015 15:05
@ryanmaxwell
Copy link

@sodastsai does your project use MTLManagedObjectAdapter? I'm having issues building the project.

I've added the following to my podfile:


pod 'MTLManagedObjectAdapter'
pod 'Overcoat', :git => 'https://github.com/sodastsai/Overcoat.git', :branch => 'mantle2'

post_install do |installer_representation|
    installer_representation.project.targets.each do |target|
        if target.name == 'Pods-Overcoat'
            target.build_configurations.each do |config|
              config.build_settings['GCC_PREPROCESSOR_DEFINITIONS'] ||= ['$(inherited)']
              config.build_settings['GCC_PREPROCESSOR_DEFINITIONS'] << 'OVERCOAT_SUPPORT_COREDATA=1'
            end
        end
    end
end

But I'm getting linker errors on compilation:

Undefined symbols for architecture x86_64:
  "_OBJC_CLASS_$_MTLManagedObjectAdapter", referenced from:
      objc-class-ref in OVCModelResponseSerializer.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

@sodastsai
Copy link
Member Author

Hmmm .... My project doesn't use CoreData. But I've tried with

  1. The test of Overcoat (which including CoreData tests)
  2. The example of Overcoat (TwitterTimeline which uses CoreData as local cache)
  3. A new project with your Podfile example
    Both are workable.

Could you try the example in the master branch and tell me your result?

@ryanmaxwell
Copy link

thanks for quick response. I can build and run the core data tests. Not sure why my project isn't building. I'll keep investigating. I suspect it may be a cocoapods integration issue.

I'm trying to include it now with the subspec 'Overcoat/CoreData' but having the same linker issue. I'll update here if I figure it out

@ryanmaxwell
Copy link

@sodastsai I've created PR which fixed my integration #91

@sodastsai
Copy link
Member Author

okay, I see. I guess you have use_frameworks! in other lines of your Podfile, right?

I've reproduced your issue now. As you said, it's related to how Cocoapods generate module map for Swift. I would fix this later. 👍

@ryanmaxwell
Copy link

yep I'm using frameworks as i've got a shared swift/obj-c project and I can @import Overcoat and import Overcoat to get the module.

@sodastsai
Copy link
Member Author

@ryanmaxwell

So, I realize one thing that original design in this commit is not only breaks Swift integration, but also bad. Yup, the interface of classes maybe changed by other external configurations.

I mean, if someone just installs just Overcoat without CoreData but he also sets OVERCOAT_SUPPORTS_COREDATA, then he would get the interface of methods related to CoreData but no implementations.

Thus I re-organize the whole interface, take CoreData methods into subclasses. (So OVCHTTPRequestOperatonManager only supports Mantle JSON and AFNetworking. And OVCManagedHTTPRequestOperationManager adds CoreData supports)

By doing so, it also solves your issues. Would you like to try and test for the master branch? (But there's a break change. You have to change class names.)

Btw, you could start test by following Podfile lines

# Work with Mantle 2.x
pod 'Overcoat/CoreData', :git => 'https://github.com/Overcoat/Overcoat.git', :branch => 'master'
# Work with Mantle 1.x
pod 'Overcoat/CoreData/Mantle1', :git => 'https://github.com/Overcoat/Overcoat.git', :branch => 'master'

(you don't have to specify MTLManagedObjectAdapter explicitly.

Thx.

@ryanmaxwell
Copy link

@sodastsai fantastic dude, pod installed master, compiled and ran no problem. Good job. (Mantle 2.x)

My only question - the interface changed from initWithBaseURL:managedObjectContext:sessionConfiguration to just initWithBaseURL:managedObjectContext:. I was passing [NSURLSessionConfiguration defaultSessionConfiguration] anyway but not sure if other people make use of that?

@sodastsai
Copy link
Member Author

which class? I cannot find this.

@ryanmaxwell
Copy link

The designated initializer for OVCManagedHTTPRequestOperationManager is now - (instancetype)initWithBaseURL:(NSURL *)url managedObjectContext:(NSManagedObjectContext *)context; whereas previously for OVCHTTPSessionManager it was - (id)initWithBaseURL:(NSURL *)url managedObjectContext:(NSManagedObjectContext *)context sessionConfiguration:(NSURLSessionConfiguration *)configuration;

@sodastsai
Copy link
Member Author

So if you want to use CoreData support with NSURLSession, you should use OVCManagedHTTPSessionManager.

OVCManagedHTTPRequestOperationManager provides CoreData support to NSURLRequest based connections.

@ryanmaxwell
Copy link

ah right. I forgot there is 2 AFNetworking class hierarchies for with and without NSURLSession. Thanks for the clarification

@sodastsai
Copy link
Member Author

👍

@sodastsai
Copy link
Member Author

@gonzalezreal I think this PR is ready (though it has been merged back already). Could you add me as a author of the cocoapods podspec? Thanks in advance.

@gonzalezreal
Copy link
Contributor

@sodastsai No problem, but I need an email address to do that

@gonzalezreal
Copy link
Contributor

@sodastsai Got the email, thanks. You need to register with that email using pod trunk so that I can add you as an owner of the pod.

@sodastsai
Copy link
Member Author

👍 already done

$ pod trunk me
  - Name:     sodastsai
  - Email:    sodas2002@gmail.com
  - Since:    August 19th, 2014 10:15

@gonzalezreal
Copy link
Contributor

Done. There was a typo in the email on the first comment.

@sodastsai
Copy link
Member Author

👍

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.

None yet

4 participants