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

Swift Package Manager compatibility #37

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ddorizy
Copy link

@ddorizy ddorizy commented Jul 2, 2019

Added Swift Package Manager manifest file for RxWebKit

Copy link
Member

@freak4pc freak4pc left a comment

Choose a reason for hiding this comment

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

Thanks! A few comments.

.package(url: "https://github.com/ReactiveX/RxSwift", from: "4.5.0")
],
targets: [
.target(name: "RxWebKit", dependencies: ["RxSwift", "RxCocoa"], path: "RxWebKit")
Copy link
Member

Choose a reason for hiding this comment

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

RxWebKit has tests - the Package.swift file should have a test target as well

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure I understand, tests targets should not be exposed in the SPM manifest file should they?

Copy link
Member

Choose a reason for hiding this comment

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

AFAIK Package.swift targets are used for public targets but test targets are only for running “Swift test” etc.

Package.swift Outdated
.library(name: "RxWebKit", targets: ["RxWebKit"])
],
dependencies: [
.package(url: "https://github.com/ReactiveX/RxSwift", from: "4.5.0")
Copy link
Member

Choose a reason for hiding this comment

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

This needs to target 5.0.1

Copy link
Author

Choose a reason for hiding this comment

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

Changed, thank you!

@alefr
Copy link

alefr commented Oct 19, 2019

Just wanted to check the status on this Pull Requets.
It's been open for a while, is it something that is planned for merge?

.library(name: "RxWebKit", targets: ["RxWebKit"])
],
dependencies: [
.package(url: "https://github.com/ReactiveX/RxSwift", from: "5.0.1")

Choose a reason for hiding this comment

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

Suggested change
.package(url: "https://github.com/ReactiveX/RxSwift", from: "5.0.1")
.package(url: "https://github.com/ReactiveX/RxSwift", from: "5.0.1"),
.package(url: "https://github.com/Quick/Quick.git", from: "2.2.0"),
.package(url: "https://github.com/Quick/Nimble.git", .branch("master"))
]

Choose a reason for hiding this comment

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

.package(url: "https://github.com/ReactiveX/RxSwift", from: "5.0.1")
],
targets: [
.target(name: "RxWebKit", dependencies: ["RxSwift", "RxCocoa"], path: "RxWebKit")

Choose a reason for hiding this comment

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

Suggested change
.target(name: "RxWebKit", dependencies: ["RxSwift", "RxCocoa"], path: "RxWebKit")
.target(name: "RxWebKit", dependencies: ["RxSwift", "RxCocoa"], path: "RxWebKit"),
.testTarget(name: "RxWebKitTests", dependencies: ["RxWebKit", "RxSwift", "RxCocoa", "RxTest", "Quick", "Nimble"], path: "RxWebKitTests")

import PackageDescription

let package = Package(
name: "RxWebKit",

Choose a reason for hiding this comment

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

Suggested change
name: "RxWebKit",
name: "RxWebKit",
platforms: [.iOS(.v8)],

@freak4pc
Copy link
Member

Is anyone interested in finishing the work on this?

@ikait
Copy link

ikait commented Feb 14, 2020

Hi @freak4pc, I would like to take over this PR and could you review #41?

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.

5 participants