diff --git a/tests/NoSimpleLetBody.elm b/tests/NoSimpleLetBody.elm index 597fd10e..aa9a3bae 100644 --- a/tests/NoSimpleLetBody.elm +++ b/tests/NoSimpleLetBody.elm @@ -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. +🔧 Running with `--fix` will automatically remove most of the reported errors. + config = [ 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. 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 @@ -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. -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 @@ -79,28 +79,13 @@ elm-review --template jfmengels/elm-review-code-style/example --rules NoSimpleLe -} rule : Rule rule = - Rule.newModuleRuleSchemaUsingContextCreator "NoSimpleLetBody" initialContext - |> Rule.withExpressionEnterVisitor (\node context -> ( expressionVisitor node context, context )) + Rule.newModuleRuleSchema "NoSimpleLetBody" () + |> Rule.withSimpleExpressionVisitor expressionVisitor |> Rule.fromModuleRuleSchema -type alias Context = - { extractSourceCode : Range -> String - } - - -initialContext : Rule.ContextCreator () Context -initialContext = - Rule.initContextCreator - (\extractSourceCode () -> - { extractSourceCode = extractSourceCode - } - ) - |> Rule.withSourceCodeExtractor - - -expressionVisitor : Node Expression -> Context -> List (Rule.Error {}) -expressionVisitor node { extractSourceCode } = +expressionVisitor : Node Expression -> List (Rule.Error {}) +expressionVisitor node = case Node.value node of Expression.LetExpression { declarations, expression } -> case Node.value expression of @@ -110,7 +95,7 @@ expressionVisitor node { extractSourceCode } = { previousEnd : Maybe Location , lastEnd : Maybe Location , last : Maybe { name : String, declarationRange : Range, expressionRange : Range } - , foundDeclaredWithName : Maybe { declarationRange : Range, expressionRange : Range, takesArguments : Bool } + , foundDeclaredWithName : Bool } declarationData = List.foldl @@ -134,16 +119,7 @@ expressionVisitor node { extractSourceCode } = else Nothing - , foundDeclaredWithName = - 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 + , foundDeclaredWithName = foundDeclaredWithName || Node.value functionDeclaration.name == name } Expression.LetDestructuring _ _ -> @@ -156,63 +132,50 @@ expressionVisitor node { extractSourceCode } = { previousEnd = Nothing , lastEnd = Nothing , last = Nothing - , foundDeclaredWithName = Nothing + , foundDeclaredWithName = False } declarations in - case declarationData.foundDeclaredWithName of - Just { declarationRange, expressionRange, takesArguments } -> - [ Rule.errorWithFix - { 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." - ] - } - (Node.range expression) - (case declarationData.last of - Just last -> - if not takesArguments then - if last.name == name then - case declarationData.previousEnd of - Nothing -> - -- It's the only element in the destructuring, we should remove move of the let expression - [ 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) + if declarationData.foundDeclaredWithName then + [ Rule.errorWithFix + { 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." + ] + } + (Node.range expression) + (case declarationData.last of + Just last -> + if last.name == name then + case declarationData.previousEnd of + Nothing -> + -- It's the only element in the destructuring, we should remove move of the let expression + [ Fix.removeRange { start = (Node.range node).start, end = last.expressionRange.start } + , Fix.removeRange { start = last.expressionRange.end, end = (Node.range node).end } ] - 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 -> - if not takesArguments then - [ Fix.removeRange declarationRange - , Fix.replaceRangeBy (Node.range expression) (extractSourceCode expressionRange) - ] + else + [] - else - [] - ) - ] + Nothing -> + [] + ) + ] - Nothing -> - [] + else + [] _ -> [] diff --git a/tests/NoSimpleLetBodyTest.elm b/tests/NoSimpleLetBodyTest.elm index c7c69d13..9cbde27a 100644 --- a/tests/NoSimpleLetBodyTest.elm +++ b/tests/NoSimpleLetBodyTest.elm @@ -8,6 +8,14 @@ import Test exposing (Test, describe, test) all : Test all = 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" <| \() -> """module A exposing (..) @@ -74,11 +82,6 @@ a = let , under = "b" } |> 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)" <| \() -> @@ -99,11 +102,6 @@ a = let , under = "b" } |> 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" <| \() -> @@ -160,3 +158,38 @@ a = let b = 1 |> Review.Test.run rule |> 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 +""" + ] + ]