-
Notifications
You must be signed in to change notification settings - Fork 33
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
As/tv os #49
As/tv os #49
Conversation
…vsimulator in ConsistencyManager target in project file
So first of all, thanks for the PR. Good point with |
Going to pull this down locally and take a look at the changes. |
👌 |
Going to look over the code. Mostly looks good but a couple of comments so far:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also make it so that we run tests on tvOS. you need to edit the build.sh
and add another clean derived data and run tests on the right sdk. Something like:
rm -rf $DERIVED_DATA &&
time xcodebuild clean test \
-project ConsistencyManager.xcodeproj \
-scheme ConsistencyManager \
-sdk macosx \ <--- this should be tvOS though? or something?
-derivedDataPath $DERIVED_DATA \
| tee build.log \
| xcpretty &&
We don't test the demo app with travis btw, so just the main project.
@@ -3,6 +3,7 @@ | |||
[![Build Status](https://travis-ci.org/linkedin/ConsistencyManager-iOS.svg?branch=master)](https://travis-ci.org/linkedin/ConsistencyManager-iOS) | |||
[![codecov](https://codecov.io/gh/linkedin/ConsistencyManager-iOS/branch/master/graph/badge.svg)](https://codecov.io/gh/linkedin/ConsistencyManager-iOS) | |||
[![GitHub release](https://img.shields.io/github/release/linkedin/ConsistencyManager-iOS.svg?maxAge=86400)](https://github.com/linkedin/ConsistencyManager-iOS/releases) | |||
![Platforms](https://img.shields.io/badge/platforms-iOS%20%7C%20tvOS%20%7C%20macOS-blue.svg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. Thanks.
|
||
import XCTest | ||
|
||
class ConsistencyManagerDemo_tvOSUITests: XCTestCase { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove these UI tests? I highly doubt we'll ever write UI tests for the demo app and it's just cruft.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably do it for xcodebuild: rm -rf $DERIVED_DATA && time xcodebuild clean build \ -project ConsistencyManager.xcodeproj \ -scheme ConsistencyManager \ -sdk appletvsimulator10.0 \ -derivedDataPath $DERIVED_DATA \ -destination 'platform=tvOS Simulator,name=Apple TV 1080p,OS=10.0' \ | tee build.log \ | xcpretty &&
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a couple of comments on the code too, but all minor stuff. Then can merge this in. :D |
I'll patch these up and push 👍 |
… test class, added tvOS xcodebuild in build.sh
I'm not seeing the empty scheme for ConsistencyManager in the SampleApp project. Is it still in there after this push? |
I'll check on the empty scheme, but don't worry about that either way. It may be a local thing, and we can delete that later. Checking changes. |
Yep. LGTM. Merging. |
🎉 Thanks! |
Summation of pull request
This PR is intends to add tvOS support to ConsistencyManager-iOS (which may have a confusing naming scheme if made multi-platform?)
Project additions
ConsistencyManager project:
SampleApp / ConsistencyManagerDemo project:
Cocoapods:
Misc:
Demo app for tvOS:
Cool tidbits
Per http://promisekit.org/news/2016/08/Multiplatform-Single-Scheme-Xcode-Projects/, including tvOS support in the ConsistencyManager target.