search

ndmitchell / hlint

Haskell source code suggestions
Other
1.48k stars 195 forks source link

rename three of the hints introduced in #1481 #1486

Closed jvoigtlaender closed 10 months ago

jvoigtlaender commented 1 year ago

Sorry, I think the name Move concatMap out was not so good after all. For example, it's not like in catMaybes (concatMap f x) ==> concatMap (catMaybes . f) x the concatMap-call is afterwards "outside" a catMaybes call.

The new names proposed now instead emphasize the function that is moving from outside into the higher-order argument of concatMap. A bit in the spirit of the name of this existing hint: https://github.com/ndmitchell/hlint/blob/e05030a08b238ff4d196338d01be1db37e891341/data/hlint.yaml#L122

jvoigtlaender commented 1 year ago

Also, what do you think of the following three rules?

    - warn: {lhs: catMaybes (concat x), rhs: concatMap catMaybes x}
    - hint: {lhs: filter f (concat x), rhs: concatMap (filter f) x}
    - warn: {lhs: mapMaybe f (concat x), rhs: concatMap (mapMaybe f) x}

All three should be efficiency improvements (since stuff is potentially filtered away before concatenating, so there is less work to do for ++, due to shorter lists).

jvoigtlaender commented 1 year ago

Also possible would be:

    - hint: {lhs: "concat (fromMaybe [] x)", rhs: "maybe [] concat x"}
    - warn: {lhs: fold (concat x), rhs: foldMap fold x}
    - warn: {lhs: foldMap f (concat x), rhs: foldMap (foldMap f) x}
ndmitchell commented 10 months ago

Sorry for the delay in getting to this. The renamings look good.

As for the other hints, the three moving to use concatMap look great. The one moving to maybe [] concat x looks reasonable. I worry about the ones that merge concat with fold are a bit more complex? In particular foldMap (foldMap f) seems a relatively complex outcome?