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 Property.filter #586

Merged
merged 2 commits into from
Feb 9, 2018
Merged

Conversation

iv-mexx
Copy link
Contributor

@iv-mexx iv-mexx commented Dec 29, 2017

Checklist

  • Updated CHANGELOG.md.

A filter operator for Property has one edge case: Property always needs to have a value, but what if the predicate excludes the current (or all) values of the source Property?
Probably thats the reason that this operator was not yet included for Property?

I suggest to provide an initial value for the filtered Property to handle that case.

Copy link
Member

@andersio andersio left a comment

Choose a reason for hiding this comment

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

Back in the times when introducing property composition, I had considered having these lifted to output optionals, but apparently that would be nasty to consume.

I'd approve but we probably need more eyes on this, since it'd set a precedence for future property operators. cc @ReactiveCocoa/reactiveswift

CHANGELOG.md Outdated
@@ -1,6 +1,8 @@
# master
*Please add new entries at the top.*

1. `Property`s now have a `filter` operator. (#586, kudos to @iv-mexx)
Copy link
Member

Choose a reason for hiding this comment

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

[Nit] New property operator: filter. 😛

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

///
/// - returns: A property that holds only values from `self` passing the given predicate.
public func filter(initial: Value, _ predicate: @escaping (Value) -> Bool) -> Property<Value> {
return Property.init(initial: initial, then: self.producer.filter(predicate))
Copy link
Member

Choose a reason for hiding this comment

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

Remove .init.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@andersio
Copy link
Member

andersio commented Dec 29, 2017

As a side note though, we often deal with streams of things we do not own (a la non instantiatable). In those cases optionality is a must, and we could do it in two ways:

  1. having operators to output Property<U?>.

    let resource: Property<UniqueResource>
    resource.filter { $0.hasExpired } // Property<UniqueResource?>
  2. apply map to optionalise, and having these operators working on opaque Value with mandatory initial value. i.e. this PR.

    let resource: Property<UniqueResource>
    resource
        .map(Optional.init)
        .filter(initial: nil) { $0.hasExpired } // Property<UniqueResource?>

Going after (1) hampers simple use cases, when there is an obvious initial value, e.g. some sort of flag derived from the content.

Meanwhile, despite (2) hampering cases with mandatory optionality in favour of simple uses, it is also IMO more explicit.

@iv-mexx
Copy link
Contributor Author

iv-mexx commented Dec 30, 2017

I had considered having these lifted to output optionals, but apparently that would be nasty to consume.

I had considered that as well at first, but it was indeed awkward to use.

@mdiep
Copy link
Contributor

mdiep commented Dec 31, 2017

I'm curious why you found it awkward to use. Returning a Property<U?> seems like it might make more sense to me. You could always do property.filter(condition).map { $0 ?? initialValue }.

It does seem like either way would get painful if you were going to chain multiple things together:

property
    .filter(initial: 1, condition1)
    .filter(initial: 1, condition2)

It'd be nice if we had could do something to propagate that through with some sentinel value. But I don't think that possible with the current APIs. 🤔

@iv-mexx
Copy link
Contributor Author

iv-mexx commented Jan 2, 2018

"Awkward" may be the wrong word for it.

Basically, it feels strange to me to introduce an optional, when "just" the initial value is missing, something that I have to deal with regardless of the filter operator already:

I often have (Mutable) properties that will always have a value, but a first "valid" value is not available at initialization. Since the value will not change back to nil, using an optional fells wrong and is often tedious down the line, so I have to find a suitable initial value anyway, e.g. empty string, empty array, ...
Now using a filter operator on such a property feels pretty similar. Sure, there are now more cases where a value might not be passed on so the property might keep the initial value longer / for ever, but once a value passes through, it does not change back to nil anymore. And since I had to choose a suitable (non-optional) initial value for the initial property, I can use the same initial value for the filter operator.

It'd be nice if we had could do something to propagate that through with some sentinel value.

This would be really great in my described use case but goes way over my head implementation wise 😆

You could always do property.filter(condition).map { $0 ?? initialValue }.

Yeah,i f you think filter outputting an optional is the correct to go than I have no problem with doing that. In the case @andersio mentioned that might be the better option anyway...

@mdiep
Copy link
Contributor

mdiep commented Jan 2, 2018

I think I'm on board with passing in the initial value like you have here. I don't see a truly better option.

@jjoelson
Copy link
Contributor

jjoelson commented Jan 3, 2018

Another (possibly stupid) idea would be to have the new property always start with the current value of the existing property and then filter all subsequent values. I think this would make the operator pretty seamless to use in most cases, though it might make it too easy to make a mistake.

@mdiep mdiep merged commit 55ac251 into ReactiveCocoa:master Feb 9, 2018
@mdiep
Copy link
Contributor

mdiep commented Feb 9, 2018

Thanks @iv-mexx!

@iv-mexx iv-mexx deleted the feature/propertyFilter branch February 9, 2018 17:39
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.

4 participants