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

AllSatisfy operator #96

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

vitallica
Copy link

  • Added an AllSatisfy operator.
  • Added tests.
  • Updated README.

This is my first contribution to this library, so please feel free to offer any feedback.

- Added an `AllSatisfy` operator.
- Added tests.
- Updated `README`.
@freak4pc
Copy link
Member

freak4pc commented Aug 3, 2021

Hey @vitallica ... It seems this operator already exists?
https://developer.apple.com/documentation/combine/publishers/allsatisfy

@vitallica
Copy link
Author

Hey @freak4pc, thanks for looking at this PR.

The native operator does not handle situations where Output is of type Collection, since the predicate takes a value of type Self.Output. This operator has a predicate that takes a value of type Output.Element.

@vitallica
Copy link
Author

vitallica commented Aug 3, 2021

@freak4pc while investigating further, I found a case in which the compiler chooses the native operator rather than this one.

let names = ["John", "Jane", "Jim", "Jill", "Joan"]

Just(names)
  .allSatisfy { $0.count <= 4 }
  .sink { print("all short? \($0)") }

This emits false, because the array itself has a count of 5. However it's possible to force the overload like so:

Just(names)
  .allSatisfy { (name: String) in name.count <= 4 }
  .sink { print("all short? \($0)") }

This emits true, because all of the elements within the array have count <= 4.

In order to avoid this conflict, I think I should rename this operator to AllSatisfyMany to match the naming convention of CombineLatestMany, MapMany, and MergeMany. What do you think?

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.

2 participants