diff --git a/tests/NoSimpleLetBody.elm b/tests/NoSimpleLetBody.elm index 7c6d3cb5..66a3e023 100644 --- a/tests/NoSimpleLetBody.elm +++ b/tests/NoSimpleLetBody.elm @@ -8,6 +8,8 @@ module NoSimpleLetBody exposing (rule) import Elm.Syntax.Expression as Expression exposing (Expression) import Elm.Syntax.Node as Node exposing (Node) +import Elm.Syntax.Range exposing (Location, Range) +import Review.Fix as Fix import Review.Rule as Rule exposing (Rule) @@ -89,25 +91,53 @@ expressionVisitor node = case Node.value expression of Expression.FunctionOrValue [] name -> let - declared : List String - declared = - List.filterMap - (\declaration -> + declarationData : + { previousEnd : Maybe Location + , lastEnd : Maybe Location + , last : Maybe { name : String, declarationRange : Range, expressionRange : Range } + , foundDeclaredWithName : Bool + } + declarationData = + List.foldl + (\declaration { lastEnd, foundDeclaredWithName } -> case Node.value declaration of Expression.LetFunction function -> - function.declaration - |> Node.value - |> .name - |> Node.value - |> Just + let + functionDeclaration : Expression.FunctionImplementation + functionDeclaration = + Node.value function.declaration + in + { previousEnd = lastEnd + , lastEnd = Just (Node.range functionDeclaration.expression).end + , last = + if List.isEmpty functionDeclaration.arguments then + Just + { name = Node.value functionDeclaration.name + , declarationRange = Node.range declaration + , expressionRange = Node.range functionDeclaration.expression + } + + else + Nothing + , foundDeclaredWithName = foundDeclaredWithName || Node.value functionDeclaration.name == name + } Expression.LetDestructuring _ _ -> - Nothing + { previousEnd = lastEnd + , lastEnd = Just (Node.range declaration).end + , last = Nothing + , foundDeclaredWithName = foundDeclaredWithName + } ) + { previousEnd = Nothing + , lastEnd = Nothing + , last = Nothing + , foundDeclaredWithName = False + } declarations in - if List.member name declared then - [ Rule.error + 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." @@ -115,6 +145,33 @@ expressionVisitor node = ] } (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 } + ] + + 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 + [] + + Nothing -> + [] + ) ] else diff --git a/tests/NoSimpleLetBodyTest.elm b/tests/NoSimpleLetBodyTest.elm index ee84b14d..d876d3bb 100644 --- a/tests/NoSimpleLetBodyTest.elm +++ b/tests/NoSimpleLetBodyTest.elm @@ -25,6 +25,95 @@ a = let b = 1 , under = "b" } |> Review.Test.atExactly { start = { row = 3, column = 8 }, end = { row = 3, column = 9 } } + |> Review.Test.whenFixed """module A exposing (..) +a = 1 +""" + ] + , test "should report an error when let body is a simple function or value and is the last declaration" <| + \() -> + """module A exposing (..) +a = let + c = 2 + b = c + in b +""" + |> 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 = let + c = 2 + in + c +""" + ] + , test "should report an error but not suggest a fix when value is not the last declaration" <| + \() -> + """module A exposing (..) +a = let + b = c + c = 2 + in b +""" + |> 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 } } + ] + , test "should report an error but not suggest a fix when value is not the last declaration (last is destructuring)" <| + \() -> + """module A exposing (..) +a = let + b = c + {c} = 2 + in b +""" + |> 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 } } + ] + , test "should report an error but not suggest a fix when value is a function that takes arguments" <| + \() -> + """module A exposing (..) +a = let + c = 2 + b n = c + in b +""" + |> 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 } } ] , test "should not report an error when let body is a function call" <| \() -> diff --git a/tests/NoUnnecessaryTrailingUnderscore.elm b/tests/NoUnnecessaryTrailingUnderscore.elm index 7b1169f5..a3a631c0 100644 --- a/tests/NoUnnecessaryTrailingUnderscore.elm +++ b/tests/NoUnnecessaryTrailingUnderscore.elm @@ -298,6 +298,9 @@ reservedElmKeywords = , "exposing_" , "as_" , "port_" + + -- Until `elm-format` and `elm-syntax` allow `infix` as an identifier + , "infix_" ]