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

[Vote] Naming for Operators that target Collections/Arrays #168

Open
freak4pc opened this issue Jul 19, 2018 · 19 comments
Open

[Vote] Naming for Operators that target Collections/Arrays #168

freak4pc opened this issue Jul 19, 2018 · 19 comments
Labels

Comments

@freak4pc
Copy link
Member

freak4pc commented Jul 19, 2018

We're trying to figure out a good naming scheme for operators that target a collection of something.

For example, like our existing mapMany operator. One of the newer ones is bind(to: [..]), vs. bindMany(...).

The big question is, should we keep on doing *many, or should we use overloads. The cost of overloads mainly comes down to performance, indexing, collision-issues, etc.

Please cast your vote!


@polls polls bot added the Polls label Jul 19, 2018
@freak4pc
Copy link
Member Author

@RxSwiftCommunity/contributors - Feedback would be highly appreciated!

@icanzilb
Copy link
Member

I personally feel that bind(to:) and drive(_) have way too many overloads already, which unnecessarily slows down compilation times. I'd also rather go with something like mapAll(_), bindAll(_), etc.

@freak4pc
Copy link
Member Author

We already have mapMany, so that might need a rename if All would be preferred. I'll do a consecutive poll after this one is done :) Thanks for the feedback!

@fpillet
Copy link
Member

fpillet commented Jul 20, 2018

One thing that would have been very nice is that bind(to: ...), drive(...) etc be made to accept a variable number of observers. I assume the conflicts with the base implementation will make it impractical, but still the idea to have the freedom specifying any number of observers is enticing.

This said, and regardless the performance issues with tooling which may resolve themselves in a decade or two, bind(all: ...), drive(all: ...) are closer to the current Swift naming conventions.

My second choice is what @icanzilb mentioned: bindAll, driveAll, mapAll.

@freak4pc
Copy link
Member Author

@fpillet To be honest that PR (#166) defines a variadic companion. This might be a good thing to try to pursue on the main repo, possibly.

We can do it in a relatively backwards compatible way, like this:

public func bind<O: ObserverType>(to observers: O...) -> Disposable where O.E == E {
    return self.bind(to: observers)
}

public func bind<O: ObserverType>(to observers: [O]) -> Disposable where Self.E == O.E {
    switch observers.count {
    case 1:
        return self.subscribe(observers[0])
    default:
        let shared = self.share()
        let disposables = observers.map(shared.subscribe)
        return CompositeDisposable(disposables: disposables)
    }
}

Think this could possibly be a good idea for a PR on the main repo. What do you think? @fpillet

@freak4pc
Copy link
Member Author

freak4pc commented Jul 20, 2018

Opened a basic PR for discussion here:
ReactiveX/RxSwift#1702 cc/ @sgtsquiggs

@bobgodwinx
Copy link
Member

@freak4pc drive(in: ...) is missing in the polls 😁

@freak4pc
Copy link
Member Author

We can add it but it doesn’t really make sense / is aligned with swift naming convention IMO

@bobgodwinx
Copy link
Member

drive(in ... ) makes sense because you a literally driving into a container of things. It's like bind(to: ...) IMHO

@freak4pc
Copy link
Member Author

If that was the case, the original operator would be called drive(in:), like bind(to:) applies to singular elements.

I don't know any place in stdlib or Swift in general where the in: notation is used to describe a collection. It's not "a container of things", it's just a collection things.

@bobgodwinx
Copy link
Member

If have to stick strictly to swift convention we wouldn't invent anything. IMO drive(in: ...) stands out especially for code completion with a clear intent because only drive(...) is already fully overloaded.

@freak4pc
Copy link
Member Author

Yes but when it comes to naming convention it needs to convey clarity, intent, and known practices of Swift.

in: usually refers to references, or when you “look something up”, but not to describe applying something to many elements IMO.

@bobgodwinx
Copy link
Member

In that case we can use drive(to: ...) what do you think ?

@freak4pc
Copy link
Member Author

The only thing is it doesn’t sound like a proper English sentence, grammatically speaking. Which is what we should strive for.

Bind to (an) array sounds fine
Drive to (an) array sounds wrong
Drive (an) array sounds fine, which is why I think it’s worth the overload for discoverability.

In any sense, if Krunoslav would agree having the variadic option it would solve overloads since there would be a single option.

@bobgodwinx
Copy link
Member

bobgodwinx commented Jul 20, 2018

Ok then we should speak proper English then at this point. How about drive(into array: [T]) or drive(into collection: ...)

@freak4pc
Copy link
Member Author

The problem with into is that in Swift it’s usually used for “In place mutation” (see reduce(into:) )

Not meaning to be anti your suggestion, but finding a good option for drive that makes good grammatical and syntactical sense would be quite hard.

@bobgodwinx
Copy link
Member

@freak4pc not at all we're just brainstorming on names. I've exhausted my names.. but drive(onto collection: ... ) or drive(into collection: ...) could still be ok. I know reduce is a combination, but at the same time it works with Sequence ... Boh 🤷🏽‍♂️ naming is HARD! 🔨

@ericmarkmartin
Copy link
Contributor

I'd like to raise the precedent that this repo set by changing map(to:) to mapTo(). In #117, @fpillet said

So this means that we will roll back to mapTo since it is not acceptable that we shadow a core operator.

@freak4pc
Copy link
Member Author

I’m aware of that precedent. Overloading works much better but is still the least preferable way to go, IMO. Unless it would be in high demand, which it doesn’t seem to be at this point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants