demystifyfp / FsToolkit.ErrorHandling

An opinionated F# Library for error handling
https://demystifyfp.gitbook.io/fstoolkit-errorhandling
MIT License
462 stars 59 forks source link

Consider using other name for Option.filter #263

Closed PawelStadnicki closed 1 month ago

PawelStadnicki commented 5 months ago

Is your feature request related to a problem? Please describe.

Option.filter meaning is not clear as it typically resides in collection modules and here we only operate on a single value. And when I understood it what it does and started using it , I had the same problem while presenting the code to my peers. They don't understand it too at a first glance.

Additionally filter even with collections provides confusion, as what indeed it does is filtering OUT item(s)

Describe the solution you'd like A better word for that function. First in mind is 'accept' but it is very subjective too.

// proceed with processing option only if something is true
amount
|> Option.accept ((=) 1000)
|> Option.either (fun _ -> "bingo") (fun _ -> "not good")

Describe alternatives you've considered

module Option =
  let accept = Option.filter

but I thing adding another name for 3rd party lib is not perfect

Additional context Downsides of the proposition: When filter is replaced it is a breaking change, when it is left with another name for the same purpose it brings confusion.

TheAngryByrd commented 5 months ago

👋 Yeah some names aren't straight forward and are chosen because of their resemblance to their list counterparts.

Additionally filter even with collections provides confusion, as what indeed it does is filtering OUT item(s)

Actually filter does filter, but it filters for the predicate value. Think of it like filtering for the thing you want. A coffee filter, filters for coffee (the liquid) and filters out anything else (the grounds).

Personally I find filter weird also and would prefer filterFor and filterOut. Would be more clear when coffee filtering what you're looking for. (filterFor (fun x -> x.IsLiquid)/filterOut(fun x -> x.AreGrounds)


That being said, I won't remove filter as that kind of breaking change is not worth it. Also, Option.filter is already part are F# core, so this really wouldn't be solved here. You're still going to need your own alias module without this library.

As far as an alias, where does pop up and people familiar with LINQ/SQL would be more comfortable. I would be open to a PR adding where in addition all the places filter current exists.

PawelStadnicki commented 5 months ago

Thank you for a very good clarification. I agree with all of that, and indeed Option.whereseems to be a better alternative. These filterFor, filterOut also sound interesting. Let me play with it in the project and then I will prepare a PR.

bartelink commented 5 months ago

TaskSeq has a where for every filter, aligning with FSharp's general stance (even though for me it's a bit of a wart, and not an overall positive to have filter and where being identical given such redundant aliases are not present in anything else in F#)

Though the coffee rationale finally gives me a way to make peace with the filter functions name, thanks!