jfmengels / elm-review-simplify

Provides elm-review rules to simplify your Elm code
https://package.elm-lang.org/packages/jfmengels/elm-review-simplify/latest/
BSD 3-Clause "New" or "Revised" License
20 stars 9 forks source link

Result.toMaybe,Maybe.map,Maybe.withDefault -> Result.map,Result.withDefault #287

Open Janiczek opened 4 months ago

Janiczek commented 4 months ago
|> Result.toMaybe
|> Maybe.map foo
|> Maybe.withDefault default

⬇️

|> Result.map foo
|> Result.withDefault default

Slack thread: https://elmlang.slack.com/archives/C010RT4D1PT/p1707473993252109 (The main result is that we should focus on the single Maybe.map first, and only handle sequences of multiple Maybe.maps as a nice-to-have later.)

What the rule should do: Remove an unneeded conversion from Result to Maybe if all we do with the Maybe later is to map+withDefault it.

Now that I write this, there is also a simpler variant without the maps:

|> Result.toMaybe
|> Maybe.withDefault default

⬇️

|> Result.withDefault default

So in its full generality, the rule should handle 0+ Maybe.maps in the same way.

What problems does it solve: Needless conversion that could be skipped

Example of things the rule would report:

0 Maybe.maps:

|> Result.toMaybe
|> Maybe.withDefault default

⬇️

|> Result.withDefault default

1 Maybe.map:

|> Result.toMaybe
|> Maybe.map foo
|> Maybe.withDefault default

⬇️

|> Result.map foo
|> Result.withDefault default

(Nice to have) 2+ Maybe.maps:

|> Result.toMaybe
|> Maybe.map foo
|> Maybe.map bar
|> Maybe.withDefault default

⬇️

|> Result.map foo
|> Result.map bar
|> Result.withDefault default

Should this be part of the Simplify rule or should it be a new rule?

Should be part of Simplify. We intentionally don't have an opinion on fusing maps together in this simplification, so it should be non-controversial.

I am looking for: