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

Add List.map on singleton simplifications #194

Closed lue-bird closed 11 months ago

lue-bird commented 11 months ago

The last unimplemented simplification from https://github.com/jfmengels/elm-review-simplify/issues/188

List.map f [ a ]
--> [ f a ]

-- below are not shown in summary for example
List.map f (if cond then [ x ] else [ y ])
--> [ f (if cond then x else y) ]

List.map f << List.singleton
--> List.singleton << f
jfmengels commented 11 months ago

Hmm, I feel a bit uneasy with this one actually. List.singleton >> List.map f -> f >> List.singleton (and the function call variants) feels very nice, but I'm thinking that List.map f [ a ] -> [ f a ] could be a bit annoying.

For instance if we imagine that f is a large lambda and a a large expression (like an if/case expression), then moving all that code can make it harder to read than it was before :thinking:

That said, this situation will be quite rare, so I'm okay to try it out. Hopefully people who don't like it will either report it back as an issue.

What do you think?

lue-bird commented 11 months ago

This applies to all kinds of fixes, e.g.

Maybe.map f (Just a) --> Just (f a)

To me this very much feels like a simplification that should always be suggested. Refactoring a possible lambda or argument into a let for example is a job for the user I'd say (or a code style elm-review rule like suggested in #147 (which I could try to create)).

[ complex
    a
    b
]
    |> List.map (\element -> { element = element, range = range })
--> simplify
[ complex
    a
    b
    |> (\element -> { element = element, range = range })
]
--> user for example
let
    element =
        complex
            a
            b
in
[ { element = element, range = range } ]
jfmengels commented 11 months ago

Alright. Yeah, it doesn't look that bad, especially since you keep the pipelines. Alright :+1: