Added assign(to: on: ownership:) operator

[dsk1306]

Using assign(to:) might produce a memory leak because it creates a strong reference to object (there is a great topic on swift forums about this). I think it might be useful to have assignWeakly(to:) operator as part of CombineExt.

[codecov]

Codecov Report

Merging #30 into master will increase coverage by 0.11%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #30      +/-   ##
==========================================
+ Coverage   97.48%   97.60%   +0.11%     
==========================================
  Files          40       42       +2     
  Lines        2387     2500     +113     
==========================================
+ Hits         2327     2440     +113     
  Misses         60       60              

Impacted Files	Coverage Δ
Sources/Operators/AssignToMany.swift	100.00% <ø> (ø)
Sources/Operators/AssignOwnership.swift	100.00% <100.00%> (ø)
Tests/AssignOwnershipTests.swift	100.00% <100.00%> (ø)
Tests/AssignToManyTests.swift	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 485d80e...dda2876. Read the comment docs.

[freak4pc]

This is great! Thank you so much. I think we had some discussions here around this, and there are two thoughts I had

1. I wonder it's worth doing something like:

public extension Publisher where Failure == Never {
    func assign<Root: AnyObject>(to keyPath: ReferenceWritableKeyPath<Root, Self.Output>,
                                 on object: Root,
                                 retainment: RetainmentStrategy) -> AnyCancellable {
        switch retainment {
        case .strong:
            return assign(to: keyPath, on: object)
        case .weak:
            return sink(receiveValue: { [weak object] value in
                object?[keyPath: keyPath] = value
            })
        }
    }
}

public enum RetainmentStrategy {
    case strong
    case weak
}

The reasoning for this is that we might need to have a lot of these weakly-suffixed operators, and I'm wondering if it's better making an overload with some specific arguments. Thoughts ? @jasdev @jdisho

2. Perhaps we should add the same for all assign overloads that accept many objects (e.g. AssignToMany.swift in the project)

Would love your thoughts :)

[dsk1306]

I think it's a great idea. I can start to work on it unless @jasdev or @jdisho have something against it.

[jasdev]

No concern on my end! I’d love to see something of this form land (I tried back when hah). There’s definitely a need for it in every day Combine code (especially ObservableObject conformances) and for what it’s worth, the CXExtensions folks think so, too.

[dsk1306]

👍

[jdisho]

Seems good to me! 🎉
Make sure to update the README too.

[jdisho]

No sure if this should be called AssignWeakly. It's not just about the weakly part.

[dsk1306]

I've renamed it to just Assign

[freak4pc]

@jasdev I know you tried, I think the thing remains the same from my angle

Does it make sense having a bunch of "new" operators called assignWeakly, sinkWeakly, whatever, or should we overload "existing" operators with an argument like I mentioned above.

I'm still divided in this, but leaning towards argument overloading. Do you, as consumers, have a preference around this? Perhaps I ask on twitter to gain some clearer understanding.

I see there's some prior art in CXExtensions so that's also something worth considering.

[jdisho]

Try making a poll in Twitter 😊

[jasdev]

Does it make sense having a bunch of “new” operators called assignWeakly, sinkWeakly, whatever, or should we overload “existing” operators with an argument like I mentioned above.

Agree that it should be an overload. 👌🏽 For the “bunch of” bit, I’m not too worried about that because the only “subscribers as operators” in Combine are sink, assign, and subscribe.

• sink already requires closure arguments, so consumers can tune capture semantics.
• subscribe comes with hint that the argument is likely strongly captured (will depend on the underlying implementation).
• assign is focusing in on a path (or the whole with \.self) and since there’s no closure arguments, it can’t handle that weak capture we’re looking for.

The only note on my end with the RetainmentStrategy is we’ll probably want give folks an RetainmentStrategy.unowned case. 😄

[dsk1306]

I wonder how many people would use it 🤔 But I agree, unowned should be a part of RetainmentStrategy.

[freak4pc]

Ok so after a relatively wide survey in one of the slack channels and some friends, let’s go with:

assign(to:on:ownership:)

ownership:
strong (alias to assign(to:on:) )
weak
unowned

@dsk1306 @jasdev @jdisho

[freak4pc]

I squashed and cleaned up a bit @dsk1306.
The only thing that I think is left is that the to-many-ownership tests don't seem to actually provide coverage, not sure why:

[freak4pc]

Tapped approve instead of changes ;O

[freak4pc]

Ok, updated the missing tests. Let me know what you think and if you want to make any additional changes before pulling this in and cutting the next version @dsk1306.

[dsk1306]

I think it would be nice to move all ownership tests from AssignToManyTests to AssignOwnershipTests. Give me a sec, I'll do it.

[freak4pc]

@dsk1306 I'm a bit iffy about this because it fits better into AssignMany than AssignOwnership. There's a bit of a fine line there, so I'm not entirely sure. Don't feel too strongly about it, I guess we can go either way.

[dsk1306]

@freak4pc I've pushed one more commit with updated unit tests. Feel free to ignore it if you think it's unnecessary.

[freak4pc]

oh cool, great idea moving these here