-
Notifications
You must be signed in to change notification settings - Fork 433
Extensible FlattenStrategy. #459
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
Conversation
Sources/Flatten.swift
Outdated
| return true | ||
|
|
||
| default: | ||
| return false |
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.
Not saying I agree with this change, but if we merge it, it doesn’t make sense to keep the Equatable conformance.
Sources/Flatten.swift
Outdated
| /// The resulting producer will complete only when all inputs have | ||
| /// completed. | ||
| public static let merge = FlattenStrategy.concurrent(limit: .max) | ||
| public static var merge: FlattenStrategy { |
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 doesn’t need to be a derived property, why not keep a constant?
|
Can you elaborate on the “without a major version bump” thing? Is that in case users are |
|
Yes, that was the reason @mdiep blocked the race strategy (#233) from merging into 1.1.x to be semver compliant. Even if extensible versioned enum (note: library evolution) eventually lands in Swift, |
|
Cool then I’m on board after those 2 comments |
|
I'll review soon, but this is a great idea. 💯 |
973ca53 to
183fd5a
Compare
In case we might want to add new flatten strategies in the future without a major version bump,
FlattenStrategynow hides its enum nature. This on paper breaks switches overFlattenStrategy, but I don't see any real use case that would be affected.Checklist
Updated CHANGELOG.md.