Open IsakSundeSingh opened 3 years ago
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 :thinking:
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 👍
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.
Great rule! Unfortunately the issue with operator precedence still exists, despite it being announced as fixed in https://github.com/truqu/elm-review-noleftpizza/issues/3.
It can be seen in this example code:
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.