Backport changes from elm-review-noleftpizza

This commit is contained in:
Jeroen Engels 2020-06-19 18:40:23 +02:00
parent a799746e40
commit b98f83d5f9
2 changed files with 643 additions and 172 deletions

View File

@ -1,8 +1,8 @@
module NoLeftPizza exposing (rule)
module NoLeftPizza exposing (rule, Strictness(..))
{-|
@docs rule
@docs rule, Strictness
-}
@ -16,6 +16,18 @@ import Review.Fix as Fix
import Review.Rule as Rule exposing (Direction, Error, Rule)
{-| Specify how strict the rule should be.
Specifying `Any` means that _any_ use of `<|` will be flagged, whereas
`Redundant` limits it to cases where `<|` can be removed - without adding any
parenthesis - without changing the semantics.
-}
type Strictness
= Any
| Redundant
{-| Forbids using the left pizza operator (<|) in infix position.
Expressions like `foo <| "hello" ++ world` will be flagged, and a fix will be
@ -23,27 +35,27 @@ proposed to write the expression to `foo ("hello" ++ world)`.
To use this rule, add it to your `elm-review` config like so:
module ReviewConfig exposing (config)
import NoLeftPizza
import Review.Rule exposing (Rule)
config : List Rule
config =
[ NoLeftPizza.rule
[ NoLeftPizza.rule NoLeftPizza.Any
]
The above configuration results in absolutely any use of `<|` being flagged. If
you'd prefer only flagging redundant usage (such as `foo <| bar`), pass
`NoLeftPizza.Redundant` as the configuration option.
If you would prefer to keep writing tests in the more "traditional" style which
uses `<|`, you can disable the rule for `tests/` like so:
module ReviewConfig exposing (config)
import NoLeftPizza
import Review.Rule exposing (Rule)
config : List Rule
config =
[ NoLeftPizza.rule
[ NoLeftPizza.rule NoLeftPizza.Any
|> Rule.ignoreErrorsForDirectories
[ -- Test functions are traditionally built up using a left pizza.
-- While we don't want them in our regular code, let's allow them
@ -52,172 +64,114 @@ uses `<|`, you can disable the rule for `tests/` like so:
]
]
Or pass `NoLeftPizza.Redundant` which will only apply to redundant usage:
import NoLeftPizza
import Review.Rule exposing (Rule)
config : List Rule
config =
[ NoLeftPizza.rule NoLeftPizza.Redundant
]
-}
rule : Rule
rule =
Rule.newModuleRuleSchema "NoLeftPizza" emptyContext
|> Rule.withDeclarationVisitor declarationVisitor
|> Rule.withExpressionVisitor expressionVisitor
rule : Strictness -> Rule
rule strictness =
Rule.newModuleRuleSchema "NoLeftPizza" strictness
|> Rule.withSimpleExpressionVisitor (expressionVisitor strictness)
|> Rule.fromModuleRuleSchema
type alias Context =
{ pizzaExpression : Maybe PizzaExpression
}
expressionVisitor : Strictness -> Node Expression -> List (Error {})
expressionVisitor strictness node =
case Node.value node of
Expression.OperatorApplication "<|" _ left right ->
case makeError strictness node left right of
Just error ->
[ error ]
Nothing ->
[]
_ ->
[]
type alias PizzaExpression =
{ node : Node Expression
, left : Node Expression
, right : Node Expression
}
makeError : Strictness -> Node Expression -> Node Expression -> Node Expression -> Maybe (Error {})
makeError strictness node left right =
case ( strictness, isSimpleExpression right ) of
( Any, False ) ->
Just (produceError strictness node left (parenthesized right))
( Redundant, False ) ->
Nothing
_ ->
Just (produceError strictness node left right)
emptyContext : Context
emptyContext =
{ pizzaExpression = Nothing
}
declarationVisitor : Node Declaration -> Direction -> Context -> ( List (Error {}), Context )
declarationVisitor _ direction context =
case direction of
Rule.OnEnter ->
( [], emptyContext )
Rule.OnExit ->
( buildErrors context, emptyContext )
expressionVisitor : Node Expression -> Direction -> Context -> ( List (Error {}), Context )
expressionVisitor node direction context =
case ( direction, Node.value node ) of
( Rule.OnExit, Expression.OperatorApplication "<|" _ left right ) ->
( buildErrors context
, { emptyContext
| pizzaExpression =
Just
{ left = left
, right = right
, node = node
}
}
)
( Rule.OnExit, Expression.OperatorApplication op dir left right ) ->
case context.pizzaExpression of
Just pizza ->
if Node.value left == Node.value pizza.node then
( [], extendPizza op dir right node pizza )
else
( buildErrors context, emptyContext )
_ ->
( [], context )
( _, _ ) ->
( [], context )
extendPizza :
String
-> InfixDirection
-> Node Expression
-> Node Expression
-> PizzaExpression
-> Context
extendPizza op dir right current pizza =
let
rightNode =
Node.Node
(Range.combine [ Node.range pizza.right, Node.range right ])
(Expression.OperatorApplication op dir pizza.right right)
newPizza =
{ left = pizza.left
, right = rightNode
, node = current
}
in
{ pizzaExpression = Just newPizza }
buildErrors : Context -> List (Error {})
buildErrors { pizzaExpression } =
pizzaExpression
|> Maybe.map (makeError >> List.singleton)
|> Maybe.withDefault []
makeError : PizzaExpression -> Error {}
makeError pizza =
Rule.errorWithFix
{ message = "That's a left pizza (<|) operator application there!"
, details =
[ "We prefer using either parenthesized function application like `Html.text (context.translate Foo.Bar)` or right pizza's like `foo |> bar`."
, "The proposed fix rewrites the expression to a simple parenthesized expression, however, this may not always be what you want. Use your best judgement!"
]
}
(Node.range pizza.node)
[ Fix.replaceRangeBy (Node.range pizza.node)
(NoLeftPizzaUtil.expressionToString (Node.range pizza.node)
(Expression.Application
[ pizza.left
, parenthesize pizza.right
]
)
produceError : Strictness -> Node Expression -> Node Expression -> Node Expression -> Error {}
produceError strictness node left right =
Rule.errorWithFix (infoFor strictness)
(Node.range node)
[ Fix.replaceRangeBy (Node.range node)
(NoLeftPizzaUtil.expressionToString (Node.range node)
(Expression.Application [ left, right ])
)
]
parenthesize : Node Expression -> Node Expression
parenthesize ((Node.Node range value) as node) =
case value of
Expression.UnitExpr ->
node
infoFor : Strictness -> { message : String, details : List String }
infoFor strictness =
case strictness of
Any ->
{ message = "That's a left pizza (<|) operator application there!"
, details =
[ "We prefer using either parenthesized function application like `Html.text (context.translate Foo.Bar)` or right pizza's like `foo |> bar`."
, "The proposed fix rewrites the expression to a simple parenthesized expression, however, this may not always be what you want. Use your best judgement!"
]
}
Expression.FunctionOrValue _ _ ->
node
Redundant ->
{ message = "Redundant left pizza (<|) operator application"
, details =
[ "This left pizza operator can be removed without any further changes, without changing the semantics of your code."
, "Using `<|` like this adds visual noise to code that can make it harder to read."
]
}
parenthesized : Node Expression -> Node Expression
parenthesized ((Node.Node range _) as node) =
Node.Node range (Expression.ParenthesizedExpression node)
isSimpleExpression : Node Expression -> Bool
isSimpleExpression (Node.Node _ expr) =
case expr of
Expression.Application _ ->
False
Expression.OperatorApplication _ _ _ _ ->
False
Expression.IfBlock _ _ _ ->
False
Expression.Operator _ ->
node
False
Expression.Integer _ ->
node
Expression.LetExpression _ ->
False
Expression.Hex _ ->
node
Expression.CaseExpression _ ->
False
Expression.Floatable _ ->
node
Expression.LambdaExpression _ ->
False
Expression.Literal _ ->
node
Expression.CharLiteral _ ->
node
Expression.TupledExpression _ ->
node
Expression.ParenthesizedExpression _ ->
node
Expression.RecordExpr _ ->
node
Expression.ListExpr _ ->
node
Expression.RecordAccess _ _ ->
node
Expression.RecordAccessFunction _ ->
node
Expression.RecordUpdateExpression _ _ ->
node
Expression.GLSLExpression _ ->
False
_ ->
Node.Node range (Expression.ParenthesizedExpression node)
True

View File

@ -1,4 +1,4 @@
module NoLeftPizzaTest exposing (..)
module NoLeftPizzaTest exposing (tests)
import NoLeftPizza
import Review.Test
@ -8,14 +8,22 @@ import Test exposing (Test, describe, test)
tests : Test
tests =
describe "NoLeftPizza"
[ anyTests
, redundantTests
]
anyTests : Test
anyTests =
describe "NoLeftPizza.Any"
[ test "Simple pizza" <|
\_ ->
"""module A exposing (..)
a = foo <| bar"""
|> Review.Test.run NoLeftPizza.rule
|> Review.Test.run (NoLeftPizza.rule NoLeftPizza.Any)
|> Review.Test.expectErrors
[ makeError "foo <| bar"
[ makeAnyError "foo <| bar"
|> Review.Test.whenFixed
"""module A exposing (..)
@ -26,15 +34,15 @@ a = foo bar"""
"""module A exposing (..)
a = foo <| bar <| baz"""
|> Review.Test.run NoLeftPizza.rule
|> Review.Test.run (NoLeftPizza.rule NoLeftPizza.Any)
|> Review.Test.expectErrors
[ makeError
[ makeAnyError
"foo <| bar <| baz"
|> Review.Test.whenFixed
"""module A exposing (..)
a = foo (bar <| baz)"""
, makeError "bar <| baz"
, makeAnyError "bar <| baz"
|> Review.Test.whenFixed
"""module A exposing (..)
@ -45,9 +53,9 @@ a = foo <| bar baz"""
"""module A exposing (..)
a = foo <| 1 + 1"""
|> Review.Test.run NoLeftPizza.rule
|> Review.Test.run (NoLeftPizza.rule NoLeftPizza.Any)
|> Review.Test.expectErrors
[ makeError
[ makeAnyError
"foo <| 1 + 1"
|> Review.Test.whenFixed
"""module A exposing (..)
@ -59,9 +67,9 @@ a = foo (1 + 1)"""
"""module A exposing (..)
a = foo <| 1 + 1 / 2"""
|> Review.Test.run NoLeftPizza.rule
|> Review.Test.run (NoLeftPizza.rule NoLeftPizza.Any)
|> Review.Test.expectErrors
[ makeError
[ makeAnyError
"foo <| 1 + 1 / 2"
|> Review.Test.whenFixed
"""module A exposing (..)
@ -75,9 +83,9 @@ a = foo (1 + 1 / 2)"""
f =
List.map .x <| y
"""
|> Review.Test.run NoLeftPizza.rule
|> Review.Test.run (NoLeftPizza.rule NoLeftPizza.Any)
|> Review.Test.expectErrors
[ makeError
[ makeAnyError
"List.map .x <| y"
|> Review.Test.whenFixed
"""module A exposing (..)
@ -89,26 +97,523 @@ f =
, test "Why isn't _this_ fixed, pt2?" <|
\_ ->
"""module A exposing (..)
f =
String.join " " <|
List.map x y
"""
|> Review.Test.run NoLeftPizza.rule
|> Review.Test.run (NoLeftPizza.rule NoLeftPizza.Any)
|> Review.Test.expectErrors
[ makeError
[ makeAnyError
"""String.join " " <|
List.map x y"""
|> Review.Test.whenFixed
"""module A exposing (..)
f =
String.join " " (List.map x y)
"""
]
, test "Why isn't _this_ fixed, pt3?" <|
\_ ->
"""module A exposing (..)
f =
String.join " " <|
List.map
(\\x ->
x
)
y
"""
|> Review.Test.run (NoLeftPizza.rule NoLeftPizza.Any)
|> Review.Test.expectErrors
[ makeAnyError
"""String.join " " <|
List.map
(\\x ->
x
)
y"""
|> Review.Test.whenFixed
"""module A exposing (..)
f =
String.join " " (List.map (\\x -> x)
y)
"""
]
, test "handle parser operators with pizza" <|
\() ->
"""
module MyParser exposing (..)
numberToken =
Parser.getChompedString <|
Parser.succeed ()
|. Parser.chompIf Char.isDigit
|. Parser.chompWhile Char.isDigit
"""
|> Review.Test.run (NoLeftPizza.rule NoLeftPizza.Any)
|> Review.Test.expectErrors
[ makeAnyError """Parser.getChompedString <|
Parser.succeed ()
|. Parser.chompIf Char.isDigit
|. Parser.chompWhile Char.isDigit"""
|> Review.Test.whenFixed
"""
module MyParser exposing (..)
numberToken =
Parser.getChompedString (Parser.succeed () |. Parser.chompIf Char.isDigit |. Parser.chompWhile Char.isDigit)
"""
]
, test "handle logic operators with pizza" <|
\() ->
"""
module A exposing (..)
f =
if isTrue <| True || False then
True
else
False
"""
|> Review.Test.run (NoLeftPizza.rule NoLeftPizza.Any)
|> Review.Test.expectErrors
[ makeAnyError "isTrue <| True || False"
|> Review.Test.whenFixed
"""
module A exposing (..)
f =
if isTrue (True || False) then
True
else
False
"""
]
, describe "mixed pizzas" mixedPizzaTests
]
mixedPizzaTests : List Test
mixedPizzaTests =
[ test "a <| (b |> c)" <|
\() ->
"""
module A exposing (..)
f =
a <| (b |> c)
"""
|> Review.Test.run (NoLeftPizza.rule NoLeftPizza.Any)
|> Review.Test.expectErrors
[ makeAnyError "a <| (b |> c)"
|> Review.Test.whenFixed
"""
module A exposing (..)
f =
a (b |> c)
"""
]
, test "(a <| b) |> c)" <|
\() ->
"""
module A exposing (..)
f =
(a <| b) |> c
"""
|> Review.Test.run (NoLeftPizza.rule NoLeftPizza.Any)
|> Review.Test.expectErrors
[ makeAnyError "a <| b"
|> Review.Test.whenFixed
"""
module A exposing (..)
f =
(a b) |> c
"""
]
, test "a |> (b <| c)" <|
\() ->
"""
module A exposing (..)
f =
a |> (b <| c)
"""
|> Review.Test.run (NoLeftPizza.rule NoLeftPizza.Any)
|> Review.Test.expectErrors
[ makeAnyError "b <| c"
|> Review.Test.whenFixed
"""
module A exposing (..)
f =
a |> (b c)
"""
]
, test "(a |> b) <| c" <|
\() ->
"""
module A exposing (..)
f =
(a |> b) <| c
"""
|> Review.Test.run (NoLeftPizza.rule NoLeftPizza.Any)
|> Review.Test.expectErrors
[ makeAnyError "(a |> b) <| c"
|> Review.Test.whenFixed
"""
module A exposing (..)
f =
(a |> b) c
"""
]
]
redundantTests : Test
redundantTests =
describe "NoLeftPizza.Redundant"
[ test "Simple pizza" <|
\_ ->
"""module A exposing (..)
a = foo <| bar"""
|> Review.Test.run (NoLeftPizza.rule NoLeftPizza.Redundant)
|> Review.Test.expectErrors
[ makeRedundantError "foo <| bar"
|> Review.Test.whenFixed
"""module A exposing (..)
a = foo bar"""
]
, test "Nested pizza" <|
\_ ->
"""module A exposing (..)
a = foo <| bar <| baz"""
|> Review.Test.run (NoLeftPizza.rule NoLeftPizza.Redundant)
|> Review.Test.expectErrors
[ makeRedundantError
"bar <| baz"
|> Review.Test.whenFixed
"""module A exposing (..)
a = foo <| bar baz"""
]
, test "Fixes operator precedence" <|
\_ ->
"""module A exposing (..)
a = foo <| 1 + 1"""
|> Review.Test.run (NoLeftPizza.rule NoLeftPizza.Redundant)
|> Review.Test.expectNoErrors
, test "Why isn't this fixed?" <|
\_ ->
"""module A exposing (..)
f =
List.map .x <| y
"""
|> Review.Test.run (NoLeftPizza.rule NoLeftPizza.Redundant)
|> Review.Test.expectErrors
[ makeRedundantError
"List.map .x <| y"
|> Review.Test.whenFixed
"""module A exposing (..)
f =
List.map .x y
"""
]
, test "Why isn't _this_ fixed, pt2?" <|
\_ ->
"""module A exposing (..)
f =
String.join " " <|
List.map x y
"""
|> Review.Test.run (NoLeftPizza.rule NoLeftPizza.Redundant)
|> Review.Test.expectNoErrors
, test "Why isn't _this_ fixed, pt3?" <|
\_ ->
"""module A exposing (..)
f =
String.join " " <|
List.map
(\\x ->
x
)
y
"""
|> Review.Test.run (NoLeftPizza.rule NoLeftPizza.Redundant)
|> Review.Test.expectNoErrors
, test "handle parser operators with pizza" <|
\() ->
"""
module MyParser exposing (..)
numberToken =
Parser.getChompedString <|
Parser.succeed ()
|. Parser.chompIf Char.isDigit
|. Parser.chompWhile Char.isDigit
"""
|> Review.Test.run (NoLeftPizza.rule NoLeftPizza.Redundant)
|> Review.Test.expectNoErrors
, test "handle logic operators with pizza" <|
\() ->
"""
module A exposing (..)
f =
if isTrue <| True || False then
True
else
False
"""
|> Review.Test.run (NoLeftPizza.rule NoLeftPizza.Redundant)
|> Review.Test.expectNoErrors
, test "Parenthesized expression" <|
\() ->
"""
module A exposing (..)
foo = bar <| (abc xyz)
"""
|> Review.Test.run (NoLeftPizza.rule NoLeftPizza.Redundant)
|> Review.Test.expectErrors
[ makeRedundantError "bar <| (abc xyz)"
|> Review.Test.whenFixed """
module A exposing (..)
foo = bar (abc xyz)
"""
]
, test "Unit expression" <|
\() ->
"""
module A exposing (..)
foo = bar <| ()
"""
|> Review.Test.run (NoLeftPizza.rule NoLeftPizza.Redundant)
|> Review.Test.expectErrors
[ makeRedundantError "bar <| ()"
|> Review.Test.whenFixed """
module A exposing (..)
foo = bar ()
"""
]
, test "Qualified function" <|
\() ->
"""
module A exposing (..)
foo = bar <| Foo.Bar.xyz
"""
|> Review.Test.run (NoLeftPizza.rule NoLeftPizza.Redundant)
|> Review.Test.expectErrors
[ makeRedundantError "bar <| Foo.Bar.xyz"
|> Review.Test.whenFixed """
module A exposing (..)
foo = bar Foo.Bar.xyz
"""
]
, test "Prefix operator" <|
\() ->
"""
module A exposing (..)
foo = bar <| (++)
"""
|> Review.Test.run (NoLeftPizza.rule NoLeftPizza.Redundant)
|> Review.Test.expectErrors
[ makeRedundantError "bar <| (++)"
|> Review.Test.whenFixed """
module A exposing (..)
foo = bar (++)
"""
]
, test "Integer value" <|
\() ->
"""
module A exposing (..)
foo = bar <| 123
"""
|> Review.Test.run (NoLeftPizza.rule NoLeftPizza.Redundant)
|> Review.Test.expectErrors
[ makeRedundantError "bar <| 123"
|> Review.Test.whenFixed """
module A exposing (..)
foo = bar 123
"""
]
, test "Floatable value" <|
\() ->
"""
module A exposing (..)
foo = bar <| 123.123
"""
|> Review.Test.run (NoLeftPizza.rule NoLeftPizza.Redundant)
|> Review.Test.expectErrors
[ makeRedundantError "bar <| 123.123"
|> Review.Test.whenFixed """
module A exposing (..)
foo = bar 123.123
"""
]
, test "Negated value" <|
\() ->
"""
module A exposing (..)
foo = bar <| -123.123
"""
|> Review.Test.run (NoLeftPizza.rule NoLeftPizza.Redundant)
|> Review.Test.expectErrors
[ makeRedundantError "bar <| -123.123"
|> Review.Test.whenFixed """
module A exposing (..)
foo = bar -123.123
"""
]
, test "String literal" <|
\() ->
"""
module A exposing (..)
foo = bar <| "hello there"
"""
|> Review.Test.run (NoLeftPizza.rule NoLeftPizza.Redundant)
|> Review.Test.expectErrors
[ makeRedundantError "bar <| \"hello there\""
|> Review.Test.whenFixed """
module A exposing (..)
foo = bar "hello there"
"""
]
, test "Character literal" <|
\() ->
"""
module A exposing (..)
foo = bar <| 'a'
"""
|> Review.Test.run (NoLeftPizza.rule NoLeftPizza.Redundant)
|> Review.Test.expectErrors
[ makeRedundantError "bar <| 'a'"
|> Review.Test.whenFixed """
module A exposing (..)
foo = bar 'a'
"""
]
, test "Tupled expression" <|
\() ->
"""
module A exposing (..)
foo = bar <| ( a, b, abc xyz )
"""
|> Review.Test.run (NoLeftPizza.rule NoLeftPizza.Redundant)
|> Review.Test.expectErrors
[ makeRedundantError "bar <| ( a, b, abc xyz )"
|> Review.Test.whenFixed """
module A exposing (..)
foo = bar (a, b, abc xyz)
"""
]
, test "Record expression" <|
\() ->
"""
module A exposing (..)
foo = bar <| { foo = 123 }
"""
|> Review.Test.run (NoLeftPizza.rule NoLeftPizza.Redundant)
|> Review.Test.expectErrors
[ makeRedundantError "bar <| { foo = 123 }"
|> Review.Test.whenFixed """
module A exposing (..)
foo = bar {foo = 123}
"""
]
, test "Record update expression" <|
\() ->
"""
module A exposing (..)
foo = bar <| { r | foo = 123 }
"""
|> Review.Test.run (NoLeftPizza.rule NoLeftPizza.Redundant)
|> Review.Test.expectErrors
[ makeRedundantError "bar <| { r | foo = 123 }"
|> Review.Test.whenFixed """
module A exposing (..)
foo = bar { r | foo = 123 }
"""
]
, test "Record access expression" <|
\() ->
"""
module A exposing (..)
foo = bar <| (foo bar).foo.bar
"""
|> Review.Test.run (NoLeftPizza.rule NoLeftPizza.Redundant)
|> Review.Test.expectErrors
[ makeRedundantError "bar <| (foo bar).foo.bar"
|> Review.Test.whenFixed """
module A exposing (..)
foo = bar (foo bar).foo.bar
"""
]
, test "Record access function" <|
\() ->
"""
module A exposing (..)
foo = bar <| .bar
"""
|> Review.Test.run (NoLeftPizza.rule NoLeftPizza.Redundant)
|> Review.Test.expectErrors
[ makeRedundantError "bar <| .bar"
|> Review.Test.whenFixed """
module A exposing (..)
foo = bar .bar
"""
]
, test "List literal" <|
\() ->
"""
module A exposing (..)
foo = bar <| [ 123, 456 ]
"""
|> Review.Test.run (NoLeftPizza.rule NoLeftPizza.Redundant)
|> Review.Test.expectErrors
[ makeRedundantError "bar <| [ 123, 456 ]"
|> Review.Test.whenFixed """
module A exposing (..)
foo = bar [123, 456]
"""
]
]
makeError : String -> Review.Test.ExpectedError
makeError under =
makeAnyError : String -> Review.Test.ExpectedError
makeAnyError under =
Review.Test.error
{ message = "That's a left pizza (<|) operator application there!"
, details =
@ -117,3 +622,15 @@ makeError under =
]
, under = under
}
makeRedundantError : String -> Review.Test.ExpectedError
makeRedundantError under =
Review.Test.error
{ message = "Redundant left pizza (<|) operator application"
, details =
[ "This left pizza operator can be removed without any further changes, without changing the semantics of your code."
, "Using `<|` like this adds visual noise to code that can make it harder to read."
]
, under = under
}