False positives with Redundant check

[IsakSundeSingh]

Great rule! Unfortunately the issue with operator precedence still exists, despite it being announced as fixed in #3.

It can be seen in this example code:

Applied from the fixes for the following errors:
NoLeftPizza: Redundant left pizza (<|) operator application

498| testEmptyString text =
499|     String.trim >> String.isEmpty <| text
500|     String.trim >> String.isEmpty text
500|

The "fix" is to remove the pipe (🍕 ) operator, but then the type of the function changes.

This happens using elm-review-noleftpizza 2.0.0 with node-elm-review 2.4.6 and elm-syntax 7.2.1.

[jfmengels]

This is not an issue about operator precedence, but rather an issue that the rule does not check what the contents on the left of the <| are.

The rule considers what is on the right to see if it's safe to remove the <|, but as shown with your example, it should also be done for the left side.

I'm guessing the left side should wrapped in parentheses when it turns out the removal of <| would change the behavior?

testEmptyString text =
- 499|     String.trim >> String.isEmpty <| text
+ 500|     (String.trim >> String.isEmpty) text

When using Redundant, I imagine it would be preferable not to report the error at all 🤔

[IsakSundeSingh]

Ah, my mistake! I just assumed it was due to operator precedence.

I guess considering the left-hand of the expression is necessary to avoid false positives, yes.
Regarding this specific example it is not really a redundant use of <| so it should probably not be reported when Redundant is used.
For the non-redundant option it seems as if your option of wrapping the left-hand side in parentheses should be the correct solution 👍

[Janiczek]

Here's another example:

String.fromInt << List.length <| someList

The rule says

This left pizza operator can be removed without any further changes, without changing the semantics of your code.

But that's not true:

String.fromInt << List.length <| someList
===
(String.fromInt << List.length) <| someList

and removing the <| would change it into

String.fromInt << List.length someList
===
String.fromInt << (List.length someList)

which doesn't compile.