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
19 stars 9 forks source link

False Positive with << #291

Closed caseyWebb closed 3 months ago

caseyWebb commented 7 months ago

Hi, I have the following function:

filterCategories : GradeLevelFilter -> List Catalog.Category -> List Catalog.Category
filterCategories gradeLevelFilter categories =
    List.filterMap
        (\(Catalog.Category category) ->
            if GradeLevelFilter.matchesList gradeLevelFilter category.gradesWithContent then
                (Just << Catalog.Category) <|
                    if List.isEmpty category.subcategories then
                        category

                    else
                        { category | subcategories = filterCategories gradeLevelFilter category.subcategories }

            else
                Nothing
        )
        categories

which erroneously reports to replace << with <|

caseyWebb commented 7 months ago

OH. remove the parens too 🤦🏼

lue-bird commented 7 months ago

Other people have reported the same thing just some time ago, so you're at the very least not alone in finding the error message+details confusing. Happy to hear ideas for improvements to the error!

caseyWebb commented 6 months ago

@lue-bird perhaps something to the effect of Unnecessary composition: This composed function is part of a pipeline. It may be added to the pipeline directly. Idk if that verbiage is self-evident either, but it probably would have made me look around the surrounding code more closely.

jfmengels commented 6 months ago

The following is mainly to clarify for other potential readers (and for me when I'm not on a computer where I can test the rule). The current error message is the following:

Simplify: Use <| instead of <<

27|             if GradeLevelFilter.matchesList gradeLevelFilter category.gradesWithContent then
28|                 (Just << Catalog.Category) <|
                          ^^
29|                     if List.isEmpty category.subcategories then

Because of the precedence of operators, using << at this location is the same as
using <|.

Please use <| instead as that is more idiomatic in Elm and generally easier to
read.

I think we can keep the short message (Use <| instead of <<) and improve the details instead, as I imagine the previous messages were thinking of as well.


How does the following sound?

Because of the precedence of operators, using << at this location is the same as
using <|.

To make it more idiomatic in Elm and generally easier to read, please use <| instead. You may need to remove some parentheses to do this.

Here is an example.
Before: data |> fn1 |> (fn2 >> fn3)
After:    data |> fn1 |> fn2 |> fn3

I'm hesitant to just say something reassuring like "Just run with --fix, you'll see that it will work out well" instead, but I imagine that this is better.

(Re-opening the issue because we do want to do something about this)