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 primary associated types #236

Closed

Conversation

danny1113
Copy link

Apple's official Combine framework also implemented primary associated types feature introduced in swift 5.7.

https://developer.apple.com/documentation/combine/connectablepublisher
https://developer.apple.com/documentation/combine/publisher
https://developer.apple.com/documentation/combine/subscriber
https://developer.apple.com/documentation/combine/subject
https://developer.apple.com/documentation/combine/scheduler

so I changed ConnectablePublisher, Publisher, Subscriber, Subject, Scheduler to support primary associated types as well.

@codecov
Copy link

codecov bot commented Nov 14, 2022

Codecov Report

Base: 97.72% // Head: 97.72% // No change to project coverage 👍

Coverage data is based on head (c36d046) compared to base (ff31c43).
Patch has no changes to coverable lines.

❗ Current head c36d046 differs from pull request most recent head d1cbb92. Consider uploading reports for the commit d1cbb92 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #236   +/-   ##
=======================================
  Coverage   97.72%   97.72%           
=======================================
  Files         105      105           
  Lines        7613     7613           
=======================================
  Hits         7440     7440           
  Misses        173      173           
Impacted Files Coverage Δ
Sources/OpenCombine/Publisher.swift 100.00% <ø> (ø)
Sources/OpenCombine/Scheduler.swift 100.00% <ø> (ø)
Sources/OpenCombine/Subject.swift 100.00% <ø> (ø)
Sources/OpenCombine/Subscriber.swift 100.00% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Kyle-Ye
Copy link

Kyle-Ye commented Nov 17, 2022

Should we add a Unit Test file for this feature in case somebody accidentally delete it?

For example:

import XCTest

#if swift(>=5.7)

#if OPENCOMBINE_COMPATIBILITY_TEST
import Combine
@available(macOS 10.15, iOS 13.0, *)
private typealias Published = Combine.Published
@available(macOS 10.15, iOS 13.0, *)
private typealias ObservableObject = Combine.ObservableObject
#else
import OpenCombine
private typealias Published = OpenCombine.Published
private typealias ObservableObject = OpenCombine.ObservableObject
#endif
@available(macOS 10.15, iOS 13.0, *)
final class PrimaryAssociatedTypeTests: XCTestCase {
    func testCombinePrimaryAssociatedTypes() {
        let p1: some Publisher<Int, Error> = Empty()
  		...
	}
}

@danny1113
Copy link
Author

Ok sure.

@Kyle-Ye
Copy link

Kyle-Ye commented Jan 4, 2023

I'll merge it as soon as I fix the failing compatibility tests.
#237 (comment)

Sorry to bother you. Is there any progress on this PR? @broadwaylamb

@NikolayJuly
Copy link

I also interested in this PR. But do we really need check for swift version?
First of all - there are tags, do we need support in older swift version? People just can ference older point in time, if they need stopped on swift 5.5 compiler, for example.
Second, it will make simplerto maintain, with only one implementation.
Am I missing anything?
I think it worth just implement primary associated types and remove swift version checks.

@danny1113
Copy link
Author

Because swift-tools-version in Package.swift is 5.5, so I think maybe we should also support Swift 5.5 as well.
Or we could bump the swift-tools-version to 5.7 and remove those version checks.

@NikolayJuly
Copy link

Tbh, I would prefer version bump in package. It will really simplify support. Less code is always better

@broadwaylamb
Copy link
Member

Support for primary associated types has been merged to master in #239. I've also published a new release. Thank you!

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.

Add primary associated type support
4 participants