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

Select vs Filter #86

Closed
DylanMeeus opened this issue May 3, 2019 · 6 comments
Closed

Select vs Filter #86

DylanMeeus opened this issue May 3, 2019 · 6 comments

Comments

@DylanMeeus
Copy link
Contributor

DylanMeeus commented May 3, 2019

Hey,

This lib looks really neat, thank you & good job! 👍

Just one thing that kind of surprised me. The Filter method is called Select here. There are quite a few languages that have the terminology for Filter. I don't really know Select from other languages, but that might be my ignorance 😄.

I believe that a lot of people will ook for a Filter method (I tried before finding out it's Select).What do you think of renaming it to Filter? (It does break the symmetry between Select and Unselect. But I also think Unselect is not a common name).

For comparison, here is a list of names for the "Filter" function in different languages: https://en.wikipedia.org/wiki/Filter_(higher-order_function).
Only mathematica seems to be using the Select terminology.

So, in short. I'd say Select -> Filter to make it easier for people to find/understand which method they need. What do you think?

I'd want to contribute this change. But I first want to know your opinion. Maybe my concern is unfounded 😅 Not to mention this will break some dependencies. Unless we can introduce a synonym and have both Filter/Select. (They can point to the same implementation, so it'd just be kind of like a "typedef").

@elliotchance
Copy link
Owner

You make a completely valid point. In fact, in early versions of pie it was actually called Only/Without for the respective Select/Unselect but I thought (as you did) that the function name would be too inconsistent and difficult to discover.

There is another recent issue #77 where combining Select with a Transform would be really helpful. I think the solution here is a Filter method because there doesn't need to be an inverse. So perhaps:

pie.Strings{"hello", "Dylan", "Meeus"}.
    Filter(func (value string) (string, bool) {
        if (unicode.IsUpper(rune(value[0]))) {
            return strings.ToUpper(value), true
        }

        return "", false
    })

// pie.Strings{"DYLAN", "MEEUS"}

Currently, this has to be done in two stages:

pie.Strings{"hello", "Dylan", "Meeus"}.
    Select(func (value string) bool {
        return unicode.IsUpper(rune(value[0]))
    }).
    Transform(strings.ToUpper)

As I write it out I realise that may be a bad example because it's kind of looks cleaner like that, but for more complex scenarios it's not as easy to split up.

What do you think?

In either case, we need to improve the documentation so it's more easily discoverable.

@DylanMeeus
Copy link
Contributor Author

Hey Elliot,

Here's my 2cents, being able to change the representation of Data in a Filter feels strange to me. (Coming from languages like Haskell and Java). This might be unexpected behaviour for someone who uses the library. If I read "Filter" I am going to assume the returned set of data is a copy of the original minus the data not matching the predicate (unaltered).

Perhaps this could be combined in something like MapFilter (What is called "Tranform" here is often called a "Map" operation. ) or with the current terminology: TranformSelect.

My idea of these kind of functions is a bit influenced by the (functional) languages I have used of course, where you can easily chain calls without reducing readability.

I do think that these type of "higher level" functions should be as self-explanatory as possible.

So to summarize, I like the idea but I disgree with the name a bit. :)

@elliotchance
Copy link
Owner

I'm not against the idea of renaming Select, Unselect and Transform. Even though it breaks the API, pie is still a new enough library that I can handle breaking a few applications now to allow a lot more in the future to work better.

Using a negator function (like pie.Not) would be difficult because it would have to pass the value through the chain which means more code generation and it would have to have a fixed API (so it would only be useful for Filter).

In your experience, would you find an opposite to Filter. What would is be called?

@DylanMeeus
Copy link
Contributor Author

DylanMeeus commented May 3, 2019

Yeah, I figured the negator function would not be as feasible. Of course the user could turn the predicate around inside the Select function, which effectively negates it. That said, I do think such a method would be interesting in the absence of a true pie.Not function.

But honestly I can't think of another language which includes this - nor can I think of a name that immediatly sounds good to me. Some suggestions off the top of my head:

  • KeepIf(..) (similar to LISPs "remove-if" and "remove-if-not")
  • NotMatching(..)
  • Excluded(..)

To just "brainstorm" and throw another idea out there.. this is something which I have seen in another language but I can't remember which one. A method to keep "both" the elements to which the predicate applies and those where it does not.

e.g

func Separate(elements []string, filter func(string) bool) (valid []string, invalid []string) {

}

I can't think of something that keeps the symmetry and sounds natural. (Purely for the symmetry Unfiltered sounds as good as Unselected).

@elliotchance
Copy link
Owner

I still like the idea of having a negating filter because I have used Unselect at least once in my production code, and I would rather not have to (in the absence of a Not method):

names.Select(func (value string) bool {
    return !actualTest(value)
})

Sometimes my mind goes off on a tangent until I realise that I've forgotten the essence of what I'm trying to answer. Not all things need to be generalised to work. I think the answer is as simple as just having a FilterNot function. It's about as explicit as I can think of and it won't get lost in the list of functions.

You have convinced me that it's worth changing these now. If you have no further comments I'm fine with:

  • Select -> Filter
  • Unselect -> FilterNot
  • Transform -> Map

Would you like to put in a PR for it?

@DylanMeeus
Copy link
Contributor Author

Hey Elliot,

I think those changes look good :)
I'll create a PR for it tonight / tomorrow!

@DylanMeeus DylanMeeus mentioned this issue May 3, 2019
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

No branches or pull requests

2 participants