jfmengels/elm-review
1
1
Fork 0
mirror of https://github.com/jfmengels/elm-review.git synced 2024-08-16 11:30:26 +03:00
Code Issues Packages Projects Releases Wiki Activity

Backport rules from elm-review-code-style

Browse Source
This commit is contained in:
Jeroen Engels 2021-08-19 20:55:58 +02:00
parent 054991e373
commit e60ff158a7
2 changed files with 90 additions and 94 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

131
tests/NoSimpleLetBody.elm
View File

@ -15,15 +15,19 @@ import Review.Rule as Rule exposing (Rule)
{-| Reports when a let expression's body is a simple reference to a value declared in the let expression. {-| Reports when a let expression's body is a simple reference to a value declared in the let expression.
🔧 Running with `--fix` will automatically remove most of the reported errors.
config = config =
[ NoSimpleLetBody.rule [ NoSimpleLetBody.rule
] ]
The reasoning behind this rule is that it is not necessary to assign a name (and type annotation) to the result of a let expression, The reasoning is that it is not necessary to assign a name to the result of a let expression,
since they are redundant with the value or function containing the expression. since they are redundant with the value or function containing the expression.
If it feels necessary to give a name anyway because it helps clarify the context, then it might be a sign that the computation of that value should be extracted to a function. If it feels necessary to give a name anyway because it helps clarify the context, then it might be a sign that the computation of that value should be extracted to a function.
Let expressions will be reported regardless of whether they're at the root of a function or deeply nested.
## Fail ## Fail
@ -63,10 +67,6 @@ The rule will not report when the referenced value was destructured in the let e
This rule resolves a minor style issue, and may not be worth enforcing depending on how strongly you feel about this issue. This rule resolves a minor style issue, and may not be worth enforcing depending on how strongly you feel about this issue.
I recommend having your full team's buy-in before enabling this rule to reduce the amount of frustration it will trigger when
it causes the tests or even the CI pipeline to fail. This rule does not (currently) provide an automatic fix which may increase
your team's frustration if they disagree with or don't care about this rule.
## Try it out ## Try it out
@ -79,28 +79,13 @@ elm-review --template jfmengels/elm-review-code-style/example --rules NoSimpleLe
-} -}
rule : Rule rule : Rule
rule = rule =
Rule.newModuleRuleSchemaUsingContextCreator "NoSimpleLetBody" initialContext Rule.newModuleRuleSchema "NoSimpleLetBody" ()
|> Rule.withExpressionEnterVisitor (\node context -> ( expressionVisitor node context, context )) |> Rule.withSimpleExpressionVisitor expressionVisitor
|> Rule.fromModuleRuleSchema |> Rule.fromModuleRuleSchema
type alias Context = expressionVisitor : Node Expression -> List (Rule.Error {})
{ extractSourceCode : Range -> String expressionVisitor node =
}
initialContext : Rule.ContextCreator () Context
initialContext =
Rule.initContextCreator
(\extractSourceCode () ->
{ extractSourceCode = extractSourceCode
}
)
|> Rule.withSourceCodeExtractor
expressionVisitor : Node Expression -> Context -> List (Rule.Error {})
expressionVisitor node { extractSourceCode } =
case Node.value node of case Node.value node of
Expression.LetExpression { declarations, expression } -> Expression.LetExpression { declarations, expression } ->
case Node.value expression of case Node.value expression of
@ -110,7 +95,7 @@ expressionVisitor node { extractSourceCode } =
{ previousEnd : Maybe Location { previousEnd : Maybe Location
, lastEnd : Maybe Location , lastEnd : Maybe Location
, last : Maybe { name : String, declarationRange : Range, expressionRange : Range } , last : Maybe { name : String, declarationRange : Range, expressionRange : Range }
, foundDeclaredWithName : Maybe { declarationRange : Range, expressionRange : Range, takesArguments : Bool } , foundDeclaredWithName : Bool
} }
declarationData = declarationData =
List.foldl List.foldl
@ -134,16 +119,7 @@ expressionVisitor node { extractSourceCode } =
else else
Nothing Nothing
, foundDeclaredWithName = , foundDeclaredWithName = foundDeclaredWithName || Node.value functionDeclaration.name == name
if Node.value functionDeclaration.name == name then
Just
{ declarationRange = Node.range declaration
, expressionRange = Node.range functionDeclaration.expression
, takesArguments = not (List.isEmpty functionDeclaration.arguments)
}
else
foundDeclaredWithName
} }
Expression.LetDestructuring _ _ -> Expression.LetDestructuring _ _ ->
@ -156,63 +132,50 @@ expressionVisitor node { extractSourceCode } =
{ previousEnd = Nothing { previousEnd = Nothing
, lastEnd = Nothing , lastEnd = Nothing
, last = Nothing , last = Nothing
, foundDeclaredWithName = Nothing , foundDeclaredWithName = False
} }
declarations declarations
in in
case declarationData.foundDeclaredWithName of if declarationData.foundDeclaredWithName then
Just { declarationRange, expressionRange, takesArguments } -> [ Rule.errorWithFix
[ Rule.errorWithFix { message = "The referenced value should be inlined."
{ message = "The referenced value should be inlined." , details =
, details = [ "The name of the value is redundant with the surrounding expression."
[ "The name of the value is redundant with the surrounding expression." , "If you believe that the expression needs a name because it is too complex, consider splitting the expression up more or extracting it to a new function."
, "If you believe that the expression needs a name because it is too complex, consider splitting the expression up more or extracting it to a new function." ]
] }
} (Node.range expression)
(Node.range expression) (case declarationData.last of
(case declarationData.last of Just last ->
Just last -> if last.name == name then
if not takesArguments then case declarationData.previousEnd of
if last.name == name then Nothing ->
case declarationData.previousEnd of -- It's the only element in the destructuring, we should remove move of the let expression
Nothing -> [ Fix.removeRange { start = (Node.range node).start, end = last.expressionRange.start }
-- It's the only element in the destructuring, we should remove move of the let expression , Fix.removeRange { start = last.expressionRange.end, end = (Node.range node).end }
[ Fix.removeRange { start = (Node.range node).start, end = last.expressionRange.start }
, Fix.removeRange { start = last.expressionRange.end, end = (Node.range node).end }
]
Just previousEnd ->
-- There are other elements in the let body that we need to keep
let
indentation : String
indentation =
String.repeat ((Node.range node).start.column - 1) " "
in
[ Fix.replaceRangeBy { start = previousEnd, end = last.expressionRange.start } ("\n" ++ indentation ++ "in\n" ++ indentation)
, Fix.removeRange { start = last.expressionRange.end, end = (Node.range node).end }
]
else
[ Fix.removeRange declarationRange
, Fix.replaceRangeBy (Node.range expression) (extractSourceCode expressionRange)
] ]
else Just previousEnd ->
[] -- There are other elements in the let body that we need to keep
let
indentation : String
indentation =
String.repeat ((Node.range node).start.column - 1) " "
in
[ Fix.replaceRangeBy { start = previousEnd, end = last.expressionRange.start } ("\n" ++ indentation ++ "in\n" ++ indentation)
, Fix.removeRange { start = last.expressionRange.end, end = (Node.range node).end }
]
Nothing -> else
if not takesArguments then []
[ Fix.removeRange declarationRange
, Fix.replaceRangeBy (Node.range expression) (extractSourceCode expressionRange)
]
else Nothing ->
[] []
) )
] ]
Nothing -> else
[] []
_ -> _ ->
[] []

53
tests/NoSimpleLetBodyTest.elm
View File

@ -8,6 +8,14 @@ import Test exposing (Test, describe, test)
all : Test all : Test
all = all =
describe "NoSimpleLetBody" describe "NoSimpleLetBody"
[ rootLevelTests
, notRootLevelTests
]
rootLevelTests : Test
rootLevelTests =
describe "At root level"
[ test "should report an error when let body is a simple function or value" <| [ test "should report an error when let body is a simple function or value" <|
\() -> \() ->
"""module A exposing (..) """module A exposing (..)
@ -74,11 +82,6 @@ a = let
, under = "b" , under = "b"
} }
|> Review.Test.atExactly { start = { row = 5, column = 8 }, end = { row = 5, column = 9 } } |> Review.Test.atExactly { start = { row = 5, column = 8 }, end = { row = 5, column = 9 } }
|> Review.Test.whenFixed """module A exposing (..)
a = let
c = 2
in c
"""
] ]
, test "should report an error but not suggest a fix when value is not the last declaration (last is destructuring)" <| , test "should report an error but not suggest a fix when value is not the last declaration (last is destructuring)" <|
\() -> \() ->
@ -99,11 +102,6 @@ a = let
, under = "b" , under = "b"
} }
|> Review.Test.atExactly { start = { row = 5, column = 8 }, end = { row = 5, column = 9 } } |> Review.Test.atExactly { start = { row = 5, column = 8 }, end = { row = 5, column = 9 } }
|> Review.Test.whenFixed """module A exposing (..)
a = let
{c} = 2
in c
"""
] ]
, test "should report an error but not suggest a fix when value is a function that takes arguments" <| , test "should report an error but not suggest a fix when value is a function that takes arguments" <|
\() -> \() ->
@ -160,3 +158,38 @@ a = let b = 1
|> Review.Test.run rule |> Review.Test.run rule
|> Review.Test.expectNoErrors |> Review.Test.expectNoErrors
] ]
notRootLevelTests : Test
notRootLevelTests =
describe "Not at root-level"
[ test "should report an error when let is deep in the function" <|
\() ->
"""module A exposing (..)
a =
if condition then
let b = 1
in b
else
2
"""
|> Review.Test.run rule
|> Review.Test.expectErrors
[ Review.Test.error
{ message = "The referenced value should be inlined."
, details =
[ "The name of the value is redundant with the surrounding expression."
, "If you believe that the expression needs a name because it is too complex, consider splitting the expression up more or extracting it to a new function."
]
, under = "b"
}
|> Review.Test.atExactly { start = { row = 5, column = 8 }, end = { row = 5, column = 9 } }
|> Review.Test.whenFixed """module A exposing (..)
a =
if condition then
1
else
2
"""
]
]