mirror of
https://github.com/jfmengels/elm-review.git
synced 2024-08-16 19:40:22 +03:00
Backport work from review-simplification
This commit is contained in:
parent
236da80392
commit
4f2500c9c8
103
tests/NoBooleanCaseOf.elm
Normal file
103
tests/NoBooleanCaseOf.elm
Normal file
@ -0,0 +1,103 @@
|
||||
module NoBooleanCaseOf exposing (rule)
|
||||
|
||||
{-|
|
||||
|
||||
@docs rule
|
||||
|
||||
-}
|
||||
|
||||
import Elm.Syntax.Expression as Expression exposing (Expression)
|
||||
import Elm.Syntax.Node as Node exposing (Node)
|
||||
import Elm.Syntax.Pattern as Pattern exposing (Pattern)
|
||||
import Review.Rule as Rule exposing (Error, Rule)
|
||||
|
||||
|
||||
{-| Reports when pattern matching is used for a boolean value.
|
||||
|
||||
The idiomatic way to check for a condition is to use an `if` expression.
|
||||
Read more about it at: <https://guide.elm-lang.org/core_language.html#if-expressions>
|
||||
|
||||
config =
|
||||
[ NoBooleanCaseOf.rule
|
||||
]
|
||||
|
||||
This won't report pattern matching when a boolean is part of the evaluated value.
|
||||
|
||||
|
||||
## Fail
|
||||
|
||||
_ =
|
||||
case bool of
|
||||
True ->
|
||||
expression
|
||||
|
||||
False ->
|
||||
otherExpression
|
||||
|
||||
|
||||
## Success
|
||||
|
||||
_ =
|
||||
if bool then
|
||||
expression
|
||||
|
||||
else
|
||||
otherExpression
|
||||
|
||||
_ =
|
||||
case ( bool, somethingElse ) of
|
||||
( True, SomeThingElse ) ->
|
||||
expression
|
||||
|
||||
_ ->
|
||||
otherExpression
|
||||
|
||||
|
||||
# When (not) to use this rule
|
||||
|
||||
You should not use this rule if you do not care about how your boolean values are
|
||||
evaluated.
|
||||
|
||||
-}
|
||||
rule : Rule
|
||||
rule =
|
||||
Rule.newModuleRuleSchema "NoBooleanCaseOf" ()
|
||||
|> Rule.withSimpleExpressionVisitor expressionVisitor
|
||||
|> Rule.fromModuleRuleSchema
|
||||
|
||||
|
||||
error : Node a -> Error {}
|
||||
error node =
|
||||
Rule.error
|
||||
{ message = "Replace `case..of` by an `if` condition"
|
||||
, details =
|
||||
[ "The idiomatic way to check for a condition is to use an `if` expression."
|
||||
, "Read more about it at: https://guide.elm-lang.org/core_language.html#if-expressions"
|
||||
]
|
||||
}
|
||||
(Node.range node)
|
||||
|
||||
|
||||
expressionVisitor : Node Expression -> List (Error {})
|
||||
expressionVisitor node =
|
||||
case Node.value node of
|
||||
Expression.CaseExpression { expression, cases } ->
|
||||
if List.any (Tuple.first >> isBoolConstructor) cases then
|
||||
[ error expression ]
|
||||
|
||||
else
|
||||
[]
|
||||
|
||||
_ ->
|
||||
[]
|
||||
|
||||
|
||||
isBoolConstructor : Node Pattern -> Bool
|
||||
isBoolConstructor node =
|
||||
case Node.value node of
|
||||
Pattern.NamedPattern { moduleName, name } _ ->
|
||||
(name == "True" || name == "False")
|
||||
&& (moduleName == [] || moduleName == [ "Basics" ])
|
||||
|
||||
_ ->
|
||||
False
|
111
tests/NoBooleanCaseOfTest.elm
Normal file
111
tests/NoBooleanCaseOfTest.elm
Normal file
@ -0,0 +1,111 @@
|
||||
module NoBooleanCaseOfTest exposing (all)
|
||||
|
||||
import NoBooleanCaseOf exposing (rule)
|
||||
import Review.Test exposing (ReviewResult)
|
||||
import Test exposing (Test, describe, test)
|
||||
|
||||
|
||||
testRule : String -> ReviewResult
|
||||
testRule string =
|
||||
"module A exposing (..)\n\n"
|
||||
++ string
|
||||
|> Review.Test.run rule
|
||||
|
||||
|
||||
message : String
|
||||
message =
|
||||
"Replace `case..of` by an `if` condition"
|
||||
|
||||
|
||||
details : List String
|
||||
details =
|
||||
[ "The idiomatic way to check for a condition is to use an `if` expression."
|
||||
, "Read more about it at: https://guide.elm-lang.org/core_language.html#if-expressions"
|
||||
]
|
||||
|
||||
|
||||
tests : List Test
|
||||
tests =
|
||||
[ test "should not report pattern matches for non-boolean values" <|
|
||||
\() ->
|
||||
testRule """
|
||||
a = case thing of
|
||||
Thing -> 1"""
|
||||
|> Review.Test.expectNoErrors
|
||||
, test "should not report pattern matches when the evaluated expression is a tuple of with a boolean" <|
|
||||
\() ->
|
||||
testRule """
|
||||
a = case ( bool1, bool2 ) of
|
||||
( True, True ) -> 1
|
||||
_ -> 2"""
|
||||
|> Review.Test.expectNoErrors
|
||||
, test "should report pattern matches when one of the patterns is a bool constructor" <|
|
||||
\() ->
|
||||
testRule """
|
||||
a = case boolTrue of
|
||||
True -> 1
|
||||
_ -> 2
|
||||
|
||||
b = case boolFalse of
|
||||
False -> 1
|
||||
_ -> 2
|
||||
|
||||
c = case boolAll of
|
||||
False -> 1
|
||||
True -> 2"""
|
||||
|> Review.Test.expectErrors
|
||||
[ Review.Test.error
|
||||
{ message = message
|
||||
, details = details
|
||||
, under = "boolTrue"
|
||||
}
|
||||
, Review.Test.error
|
||||
{ message = message
|
||||
, details = details
|
||||
, under = "boolFalse"
|
||||
}
|
||||
, Review.Test.error
|
||||
{ message = message
|
||||
, details = details
|
||||
, under = "boolAll"
|
||||
}
|
||||
]
|
||||
, test "should report pattern matches for booleans even when one of the patterns starts with `Basics.`" <|
|
||||
\() ->
|
||||
testRule """
|
||||
a = case boolTrue of
|
||||
Basics.True -> 1
|
||||
_ -> 2
|
||||
|
||||
b = case boolFalse of
|
||||
Basics.False -> 1
|
||||
_ -> 2"""
|
||||
|> Review.Test.expectErrors
|
||||
[ Review.Test.error
|
||||
{ message = message
|
||||
, details = details
|
||||
, under = "boolTrue"
|
||||
}
|
||||
, Review.Test.error
|
||||
{ message = message
|
||||
, details = details
|
||||
, under = "boolFalse"
|
||||
}
|
||||
]
|
||||
, test "should report pattern matches for booleans even when the constructor seems to be for booleans but comes from an unknown module" <|
|
||||
\() ->
|
||||
testRule """
|
||||
a = case boolTrue of
|
||||
OtherModule.True -> 1
|
||||
_ -> 2
|
||||
|
||||
b = case boolFalse of
|
||||
OtherModule.False -> 1
|
||||
_ -> 2"""
|
||||
|> Review.Test.expectNoErrors
|
||||
]
|
||||
|
||||
|
||||
all : Test
|
||||
all =
|
||||
describe "NoBooleanCaseOf" tests
|
63
tests/NoFullyAppliedPrefixOperator.elm
Normal file
63
tests/NoFullyAppliedPrefixOperator.elm
Normal file
@ -0,0 +1,63 @@
|
||||
module NoFullyAppliedPrefixOperator exposing (rule)
|
||||
|
||||
{-|
|
||||
|
||||
@docs rule
|
||||
|
||||
-}
|
||||
|
||||
import Elm.Syntax.Expression as Expression exposing (Expression)
|
||||
import Elm.Syntax.Node as Node exposing (Node)
|
||||
import Elm.Syntax.Range exposing (Range)
|
||||
import Review.Rule as Rule exposing (Error, Rule)
|
||||
|
||||
|
||||
{-| Reports when an operator is used as a prefix operator and all the operands are already given.
|
||||
|
||||
config =
|
||||
[ NoFullyAppliedPrefixOperator.rule
|
||||
]
|
||||
|
||||
|
||||
## Fail
|
||||
|
||||
_ =
|
||||
(+) 1 2
|
||||
|
||||
|
||||
## Success
|
||||
|
||||
_ =
|
||||
1 + 2
|
||||
|
||||
_ =
|
||||
(+) 1
|
||||
|
||||
_ =
|
||||
(+)
|
||||
|
||||
-}
|
||||
rule : Rule
|
||||
rule =
|
||||
Rule.newModuleRuleSchema "NoFullyAppliedPrefixOperator" ()
|
||||
|> Rule.withSimpleExpressionVisitor expressionVisitor
|
||||
|> Rule.fromModuleRuleSchema
|
||||
|
||||
|
||||
error : Range -> Error {}
|
||||
error range =
|
||||
Rule.error
|
||||
{ message = "Prefer using the infix form (`a + b`) over the prefix form (`(+) a b`) when possible"
|
||||
, details = [ "The prefix form is generally harder to read over the infix form." ]
|
||||
}
|
||||
range
|
||||
|
||||
|
||||
expressionVisitor : Node Expression -> List (Error {})
|
||||
expressionVisitor node =
|
||||
case Node.value node of
|
||||
Expression.Application [ Node.Node range (Expression.PrefixOperator _), _, _ ] ->
|
||||
[ error range ]
|
||||
|
||||
_ ->
|
||||
[]
|
96
tests/NoFullyAppliedPrefixOperatorTest.elm
Normal file
96
tests/NoFullyAppliedPrefixOperatorTest.elm
Normal file
@ -0,0 +1,96 @@
|
||||
module NoFullyAppliedPrefixOperatorTest exposing (all)
|
||||
|
||||
import NoFullyAppliedPrefixOperator exposing (rule)
|
||||
import Review.Test exposing (ReviewResult)
|
||||
import Test exposing (Test, describe, test)
|
||||
|
||||
|
||||
message : String
|
||||
message =
|
||||
"Prefer using the infix form (`a + b`) over the prefix form (`(+) a b`) when possible"
|
||||
|
||||
|
||||
details : List String
|
||||
details =
|
||||
[ "The prefix form is generally harder to read over the infix form."
|
||||
]
|
||||
|
||||
|
||||
all : Test
|
||||
all =
|
||||
describe "NoFullyAppliedPrefixOperator"
|
||||
[ test "should not report a lonely operator" <|
|
||||
\() ->
|
||||
"""
|
||||
module A exposing (..)
|
||||
a = (++)
|
||||
b = (::)
|
||||
c = (//)
|
||||
d = (+)
|
||||
e = (/)
|
||||
"""
|
||||
|> Review.Test.run rule
|
||||
|> Review.Test.expectNoErrors
|
||||
, test "should not report an operator used in infix position" <|
|
||||
\() ->
|
||||
"""
|
||||
module A exposing (..)
|
||||
a = y ++ z
|
||||
b = y :: z
|
||||
c = y // z
|
||||
d = y + z
|
||||
e = y / z
|
||||
"""
|
||||
|> Review.Test.run rule
|
||||
|> Review.Test.expectNoErrors
|
||||
, test "should not report an operator used in prefix position with one argument" <|
|
||||
\() ->
|
||||
"""
|
||||
module A exposing (..)
|
||||
a = (++) z
|
||||
b = (::) z
|
||||
c = (//) z
|
||||
d = (+) z
|
||||
e = (/) z
|
||||
"""
|
||||
|> Review.Test.run rule
|
||||
|> Review.Test.expectNoErrors
|
||||
, test "should report an operator used in prefix position with both arguments" <|
|
||||
\() ->
|
||||
"""
|
||||
module A exposing (..)
|
||||
a = (++) y z
|
||||
b = (::) y z
|
||||
c = (//) y z
|
||||
d = (+) y z
|
||||
e = (/) y z
|
||||
"""
|
||||
|> Review.Test.run rule
|
||||
|> Review.Test.expectErrors
|
||||
[ Review.Test.error
|
||||
{ message = message
|
||||
, details = details
|
||||
, under = "(++)"
|
||||
}
|
||||
, Review.Test.error
|
||||
{ message = message
|
||||
, details = details
|
||||
, under = "(::)"
|
||||
}
|
||||
, Review.Test.error
|
||||
{ message = message
|
||||
, details = details
|
||||
, under = "(//)"
|
||||
}
|
||||
, Review.Test.error
|
||||
{ message = message
|
||||
, details = details
|
||||
, under = "(+)"
|
||||
}
|
||||
, Review.Test.error
|
||||
{ message = message
|
||||
, details = details
|
||||
, under = "(/)"
|
||||
}
|
||||
]
|
||||
]
|
116
tests/NoListLiteralsConcat.elm
Normal file
116
tests/NoListLiteralsConcat.elm
Normal file
@ -0,0 +1,116 @@
|
||||
module NoListLiteralsConcat exposing (rule)
|
||||
|
||||
{-|
|
||||
|
||||
@docs rule
|
||||
|
||||
-}
|
||||
|
||||
import Elm.Syntax.Expression as Expression exposing (Expression)
|
||||
import Elm.Syntax.Node as Node exposing (Node)
|
||||
import Elm.Syntax.Range exposing (Range)
|
||||
import Review.Rule as Rule exposing (Error, Rule)
|
||||
|
||||
|
||||
{-| Reports when an operation on lists could be simplified to a single literal list.
|
||||
|
||||
config =
|
||||
[ NoListLiteralsConcat.rule
|
||||
]
|
||||
|
||||
|
||||
## Fail
|
||||
|
||||
_ =
|
||||
[ 1, 2, 3 ] ++ [ 4, mysteryNumber, 6 ]
|
||||
|
||||
_ =
|
||||
List.concat
|
||||
[ [ 1, 2, 3 ]
|
||||
, [ 4, mysteryNumber, 6 ]
|
||||
]
|
||||
|
||||
_ =
|
||||
List.concat
|
||||
[ [ 1, 2, 3 ]
|
||||
]
|
||||
|
||||
_ =
|
||||
1 :: [ 2, 3 ]
|
||||
|
||||
_ =
|
||||
[] ++ list
|
||||
|
||||
_ =
|
||||
list ++ []
|
||||
|
||||
|
||||
## Success
|
||||
|
||||
_ =
|
||||
[ 1, 2, 3, 4, mysteryNumber, 6 ]
|
||||
|
||||
_ =
|
||||
[ 1, 2, 3 ] ++ list ++ [ 4, mysteryNumber, 6 ]
|
||||
|
||||
_ =
|
||||
List.concat
|
||||
[ [ 1, 2, 3 ]
|
||||
, list
|
||||
, [ 4, mysteryNumber, 6 ]
|
||||
]
|
||||
|
||||
-}
|
||||
rule : Rule
|
||||
rule =
|
||||
Rule.newModuleRuleSchema "NoListLiteralsConcat" ()
|
||||
|> Rule.withSimpleExpressionVisitor expressionVisitor
|
||||
|> Rule.fromModuleRuleSchema
|
||||
|
||||
|
||||
error : Range -> Error {}
|
||||
error range =
|
||||
Rule.error
|
||||
{ message = "Expression could be simplified to be a single List"
|
||||
, details = [ "Try moving all the elements into a single list." ]
|
||||
}
|
||||
range
|
||||
|
||||
|
||||
expressionVisitor : Node Expression -> List (Error {})
|
||||
expressionVisitor node =
|
||||
case Node.value node of
|
||||
Expression.OperatorApplication "++" _ (Node.Node _ (Expression.ListExpr _)) (Node.Node _ (Expression.ListExpr _)) ->
|
||||
[ error (Node.range node) ]
|
||||
|
||||
Expression.OperatorApplication "++" _ (Node.Node range (Expression.ListExpr [])) _ ->
|
||||
[ error range ]
|
||||
|
||||
Expression.OperatorApplication "++" _ _ (Node.Node range (Expression.ListExpr [])) ->
|
||||
[ error range ]
|
||||
|
||||
Expression.OperatorApplication "::" _ _ (Node.Node _ (Expression.ListExpr _)) ->
|
||||
[ error (Node.range node) ]
|
||||
|
||||
Expression.Application [ Node.Node _ (Expression.FunctionOrValue [ "List" ] "concat"), Node.Node _ (Expression.ListExpr list) ] ->
|
||||
if List.length list < 2 then
|
||||
[ error (Node.range node) ]
|
||||
|
||||
else if List.all isListLiteral list then
|
||||
[ error (Node.range node) ]
|
||||
|
||||
else
|
||||
[]
|
||||
|
||||
_ ->
|
||||
[]
|
||||
|
||||
|
||||
isListLiteral : Node Expression -> Bool
|
||||
isListLiteral node =
|
||||
case Node.value node of
|
||||
Expression.ListExpr _ ->
|
||||
True
|
||||
|
||||
_ ->
|
||||
False
|
154
tests/NoListLiteralsConcatTest.elm
Normal file
154
tests/NoListLiteralsConcatTest.elm
Normal file
@ -0,0 +1,154 @@
|
||||
module NoListLiteralsConcatTest exposing (all)
|
||||
|
||||
import NoListLiteralsConcat exposing (rule)
|
||||
import Review.Test exposing (ReviewResult)
|
||||
import Test exposing (Test, describe, test)
|
||||
|
||||
|
||||
testRule : String -> ReviewResult
|
||||
testRule string =
|
||||
"module A exposing (..)\n\n"
|
||||
++ string
|
||||
|> Review.Test.run rule
|
||||
|
||||
|
||||
all : Test
|
||||
all =
|
||||
describe "NoListLiteralsConcat"
|
||||
[ usingPlusPlusTests
|
||||
]
|
||||
|
||||
|
||||
message : String
|
||||
message =
|
||||
"Expression could be simplified to be a single List"
|
||||
|
||||
|
||||
details : List String
|
||||
details =
|
||||
[ "Try moving all the elements into a single list." ]
|
||||
|
||||
|
||||
usingPlusPlusTests : Test
|
||||
usingPlusPlusTests =
|
||||
describe "Using (++)"
|
||||
[ test "should not report a single list literal" <|
|
||||
\() ->
|
||||
testRule """
|
||||
a = []
|
||||
b = [1]
|
||||
c = [ "string", "foo", "bar" ]
|
||||
"""
|
||||
|> Review.Test.expectNoErrors
|
||||
, test "should report concatenating two list literals" <|
|
||||
\() ->
|
||||
testRule """
|
||||
a = [ 1 ] ++ [ 2, 3 ]
|
||||
"""
|
||||
|> Review.Test.expectErrors
|
||||
[ Review.Test.error
|
||||
{ message = message
|
||||
, details = details
|
||||
, under = "[ 1 ] ++ [ 2, 3 ]"
|
||||
}
|
||||
]
|
||||
, test "should report concatenating two list literals, even they contain variables" <|
|
||||
\() ->
|
||||
testRule """
|
||||
a = [ a, 1 ] ++ [ b, 2 ]
|
||||
"""
|
||||
|> Review.Test.expectErrors
|
||||
[ Review.Test.error
|
||||
{ message = message
|
||||
, details = details
|
||||
, under = "[ a, 1 ] ++ [ b, 2 ]"
|
||||
}
|
||||
]
|
||||
, test "should report concatenating an empty list and something" <|
|
||||
\() ->
|
||||
testRule """
|
||||
a = [] ++ something
|
||||
"""
|
||||
|> Review.Test.expectErrors
|
||||
[ Review.Test.error
|
||||
{ message = message
|
||||
, details = details
|
||||
, under = "[]"
|
||||
}
|
||||
]
|
||||
, test "should report concatenating something and an empty list" <|
|
||||
\() ->
|
||||
testRule """
|
||||
a = something ++ []
|
||||
"""
|
||||
|> Review.Test.expectErrors
|
||||
[ Review.Test.error
|
||||
{ message = message
|
||||
, details = details
|
||||
, under = "[]"
|
||||
}
|
||||
]
|
||||
, test "should not report using :: to a variable or expression" <|
|
||||
\() ->
|
||||
testRule """
|
||||
a = 1 :: list
|
||||
b = 1 :: foo bar
|
||||
"""
|
||||
|> Review.Test.expectNoErrors
|
||||
, test "should report using :: to a list literal" <|
|
||||
\() ->
|
||||
testRule """
|
||||
a = 1 :: [ 2, 3]
|
||||
"""
|
||||
|> Review.Test.expectErrors
|
||||
[ Review.Test.error
|
||||
{ message = message
|
||||
, details = details
|
||||
, under = "1 :: [ 2, 3]"
|
||||
}
|
||||
]
|
||||
, test "should not report List.concat that contains a variable or expression" <|
|
||||
\() ->
|
||||
testRule """
|
||||
a = List.concat [ foo, bar ]
|
||||
b = List.concat [ [ 1 ], foo ]
|
||||
c = List.concat [ foo, [ 1 ] ]
|
||||
"""
|
||||
|> Review.Test.expectNoErrors
|
||||
, test "should report List.concat with no items" <|
|
||||
\() ->
|
||||
testRule """
|
||||
a = List.concat []
|
||||
"""
|
||||
|> Review.Test.expectErrors
|
||||
[ Review.Test.error
|
||||
{ message = message
|
||||
, details = details
|
||||
, under = "List.concat []"
|
||||
}
|
||||
]
|
||||
, test "should report List.concat with a single item" <|
|
||||
\() ->
|
||||
testRule """
|
||||
a = List.concat [ a ]
|
||||
"""
|
||||
|> Review.Test.expectErrors
|
||||
[ Review.Test.error
|
||||
{ message = message
|
||||
, details = details
|
||||
, under = "List.concat [ a ]"
|
||||
}
|
||||
]
|
||||
, test "should report List.concat that only contains list literals" <|
|
||||
\() ->
|
||||
testRule """
|
||||
a = List.concat [ [ 1, 2, 3 ], [ 4, 5, 6] ]
|
||||
"""
|
||||
|> Review.Test.expectErrors
|
||||
[ Review.Test.error
|
||||
{ message = message
|
||||
, details = details
|
||||
, under = "List.concat [ [ 1, 2, 3 ], [ 4, 5, 6] ]"
|
||||
}
|
||||
]
|
||||
]
|
Loading…
Reference in New Issue
Block a user