Add/remove delegate operators no longer return values

[bubski]

+= and -= used to return MulticastDelegate. By convention these operators mutate lhs and not return anything.

This came up in Swift 3 where the compiler gave me warnings about unused results, because the convention for default warning has changed.

Still, event if Swift 2 doesn't warn, I think this approach follows conventions better and is also prepared better for Swfit 3.

CollectionType example:

/// Extend `lhs` with the elements of `rhs`.
public func +=<Element, C : CollectionType where C.Generator.Element == Element>(inout lhs: [Element], rhs: C)

Int32 example

public func -=(inout lhs: Int32, rhs: Int32)

[JRG-Developer]

I think we should consider this, but I don't like doing this PR against current master as it's a non-backwards compatible change that goes against Swift 2 idioms...

I suggest we wait for Issue 9: Swift 3 branch to be resolved first and then change PR to merge into said new branch.

[jonasman]

I don't see this as breaking compatibility. @JRG-Developer did you have any issue? On my Xcode 7.3.1 it works fine .

[JRG-Developer]

I've thought about this a bit more, and here's what I came up with:

Technically, this isn't backwards compatible (since it removes a return value). However, this would only matter if a consumer wrote something like this:

var newMulticastDelegate = multicastDelegate += delegateObject

This wouldn't compile anymore if this PR is accepted on master.

However, this seems really awkward. I audited each of my consuming projects, and I didn't find this anywhere.

So, I suppose I actually am onboard with merging this into master, even as a new patch version. 😉

[bubski]

It's backwards compatible swift-wise, but not cocoapod-wise.

I'd release it as a minor update rather than patch update because the public API changes (and that's what I see you already did 👍)

Thanks for merging

[JRG-Developer]

minor or patch, either is good with me here! 🍷 😉

[jonasman]

yeap, i bumped it to 1.1 . I also released 1.1.1 because one method was not converted.

[bubski]

I omitted that one on purpose, to allow chaining. Also had clear arguments only for += and -=.

I'm not sure which version of |> i prefer.

[JRG-Developer]

IMHO, chaining here is a reasonable use case that should be supported.

For example, this seems perfectly fine to me:

multicastDelegate |> { $0.doThis() } |> { $0.doThat() }

I'd actually prefer the |> operator to return the MulticastDelegate object.

[JRG-Developer]

Note that these are different:

// doThis called and then doThat immediately called after on each delegate
multicastDelegate |> { 
    $0.doThis() 
    $0.doThat()
}

// doThis called on each and every delegate before doThat is called on any delegate
multicastDelegate |> { $0.doThis() } |> { $0.doThat() }

[JRG-Developer]

My argument for why this should be allowed is: chaining here feels Swifty.

In the case of += and -=, it doesn't feel Swifty.

😉

[jonasman]

Hm valid points. The only issue is if you don't chain you will get a warning "result unused"

Sent from my iPhone

On 05/08/2016, at 00:39, Joshua Greene notifications@github.com wrote:

My argument for why this should be allowed is: chaining here feels Swifty.

In the case of += and -=, it doesn't feel Swifty.

😉

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub, or mute the thread.

[bubski]

You would get result unused only on Swift 3, where you should annotate the method with @discardableResult to silence the warning and make clear that method has side-effects and the returned value may be implicitly ignored.

[jonasman]

I fail to see what is the advantage of chaining here, can you guys give me a good example?

[JRG-Developer]

The "advantage" of chaining is readability: at times chaining can improve readability.

The standard Swift functions- map, filter, reduce, etc- support chaining. IMHO, we'd need a good reason not to support it, as not supporting it would actually go against the Swift trend.

Whether or not you use chaining is up to you, even on a case-by-case basis, but I think it should be there.

[jonasman]

I understand the advantages of chaining. But in this case i don't see any. map doesn't chain the mapping into 2 calls but performs the map in one block. Similarly here we perform the delegate invocation in 1 call.

Can you provide an example where chaining would be more readable?

[JRG-Developer]

map, et all totally support chaining.

As a trivial example, say you have an array of US dollar amounts:

let dollarAmounts: [Float] = [1, 30, 15, 20]

However, you need to convert this to Japanese Yen string values ([String]). You could do this using chaining like this:

let yenDisplayValues = dollarAmounts.map { $0 * 100 } .map { "\($0)"}

Yes, you could combine said two maps, but again this is a trivial example 😉 , and this won't always be possible.

Besides, this is way easy to support: we just need to return the multicast delegate from the |>; it's not like we're talking about a huge feature here...

Let's support chaining so consumers have the option to use it.

If you don't see value for using chaining in your project right now, well, don't use it right now. But, it's there later if you ever do find value for it. 😉

[jonasman]

Yes i understand chaining. In this case map is not returning self but the mapping of the array. In our case we return self for no apparent reason. It is the same as if UITableView delegate would return self from any of the calls: delegate.numberOfRows(table, section).cellForRow(table,indexPath) In a delegate pattern that doesn't make sense and it is anti-pattern. That is why I was asking a concrete example where this is used.

[JRG-Developer]

Sigh, it sounds like we're bikeshedding here...

I think it's best to provide chaining here as an option, if only to support the possibility of its use by a consumer that wants it.

Frankly, though, I don't care enough about this... If you want to accept the PR, go for it. Otherwise, meh? 😜

[bubski]

Possibility to chain sounds reasonable for me too, supported by points @JRG-Developer mentioned.