jfmengels/elm-review
1
1
Fork 0
mirror of https://github.com/jfmengels/elm-review.git synced 2024-10-05 20:18:06 +03:00
Code Issues Packages Projects Releases Wiki Activity

Add withSourceCodeExtractor (#100)

Browse Source
This commit is contained in:
Jeroen Engels 2021-05-29 00:00:12 +02:00 committed by GitHub
parent 406b728455
commit 7489b6b458
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 348 additions and 59 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
docs.json
View File

File diff suppressed because one or more lines are too long

1
review/src/ReviewConfig.elm
View File

@ -55,6 +55,7 @@ config =
--, NoUnnecessaryTrailingUnderscore.rule
, NoUnused.Variables.rule
, NoSimpleLetBody.rule
]
|> List.map (Rule.ignoreErrorsForDirectories [ "src/Vendor/", "tests/Vendor/" ])
|> List.map (Rule.ignoreErrorsForFiles [ "tests/NoUnused/Patterns/NameVisitor.elm" ])

96
src/Review/Rule.elm
View File

@ -11,7 +11,7 @@ module Review.Rule exposing
, withFinalModuleEvaluation
, withElmJsonModuleVisitor, withReadmeModuleVisitor, withDependenciesModuleVisitor
, ProjectRuleSchema, newProjectRuleSchema, fromProjectRuleSchema, withModuleVisitor, withModuleContext, withModuleContextUsingContextCreator, withElmJsonProjectVisitor, withReadmeProjectVisitor, withDependenciesProjectVisitor, withFinalProjectEvaluation, withContextFromImportedModules
, ContextCreator, Metadata, initContextCreator, isInSourceDirectories, moduleNameFromMetadata, moduleNameNodeFromMetadata, withMetadata, withModuleNameLookupTable, withModuleKey
, ContextCreator, Metadata, initContextCreator, isInSourceDirectories, moduleNameFromMetadata, moduleNameNodeFromMetadata, withMetadata, withModuleNameLookupTable, withModuleKey, withSourceCodeExtractor
, Error, error, errorWithFix, ModuleKey, errorForModule, errorForModuleWithFix, ElmJsonKey, errorForElmJson, errorForElmJsonWithFix, ReadmeKey, errorForReadme, errorForReadmeWithFix
, globalError, configurationError
, ReviewError, errorRuleName, errorMessage, errorDetails, errorRange, errorFixes, errorFilePath, errorTarget
@ -218,7 +218,7 @@ first, as they are in practice a simpler version of project rules.
## Requesting more information
@docs ContextCreator, Metadata, initContextCreator, isInSourceDirectories, moduleNameFromMetadata, moduleNameNodeFromMetadata, withMetadata, withModuleNameLookupTable, withModuleKey
@docs ContextCreator, Metadata, initContextCreator, isInSourceDirectories, moduleNameFromMetadata, moduleNameNodeFromMetadata, withMetadata, withModuleNameLookupTable, withModuleKey, withSourceCodeExtractor
## Errors
@ -1183,7 +1183,7 @@ fromProjectRuleSchema ((ProjectRuleSchema schema) as projectRuleSchema) =
requestedData
Nothing ->
RequestedData { metadata = False, moduleNameLookupTable = False }
RequestedData { moduleNameLookupTable = False, sourceCodeExtractor = False }
, ruleImplementation =
\exceptions project nodeContexts ->
let
@ -1226,7 +1226,7 @@ fromProjectRuleSchemaToRunnableProjectVisitor (ProjectRuleSchema schema) =
requestedData
Nothing ->
RequestedData { metadata = False, moduleNameLookupTable = False }
RequestedData { moduleNameLookupTable = False, sourceCodeExtractor = False }
}
@ -1254,6 +1254,7 @@ mergeModuleVisitors initialProjectContext maybeModuleContextCreator visitors =
}
, moduleKey = ModuleKey "dummy"
, moduleNameLookupTable = ModuleNameLookupTableInternal.empty
, extractSourceCode = always "dummy"
}
initialModuleContext : moduleContext
@ -1389,7 +1390,7 @@ configurationError name configurationError_ =
Rule
{ name = name
, exceptions = Exceptions.init
, requestedData = RequestedData { metadata = False, moduleNameLookupTable = False }
, requestedData = RequestedData { moduleNameLookupTable = False, sourceCodeExtractor = False }
, ruleImplementation = \_ _ _ -> ( [], configurationError name configurationError_ )
, configurationError = Just configurationError_
}
@ -3961,6 +3962,16 @@ computeModules projectVisitor ( moduleVisitor, moduleContextCreator ) project ex
, moduleNameLookupTable =
Dict.get (Review.Project.Internal.getModuleName module_) moduleNameLookupTables
|> Maybe.withDefault ModuleNameLookupTableInternal.empty
, extractSourceCode =
let
(RequestedData requestedData) =
projectVisitor.requestedData
in
if requestedData.sourceCodeExtractor then
extractSourceCode (String.lines module_.source)
else
always ""
}
initialModuleContext : moduleContext
@ -4107,6 +4118,26 @@ visitModuleForProjectRule schema initialContext module_ =
|> (\( errors, moduleContext ) -> ( makeFinalEvaluation schema.finalEvaluationFns ( errors, moduleContext ), moduleContext ))
extractSourceCode : List String -> Range -> String
extractSourceCode lines range =
lines
|> List.drop (range.start.row - 1)
|> List.take (range.end.row - range.start.row + 1)
|> mapLast (String.slice 0 (range.end.column - 1))
|> String.join "\n"
|> String.dropLeft (range.start.column - 1)
mapLast : (a -> a) -> List a -> List a
mapLast mapper lines =
case List.reverse lines of
[] ->
lines
first :: rest ->
List.reverse (mapper first :: rest)
visitImport :
List (Node Import -> moduleContext -> ( List (Error {}), moduleContext ))
-> Node Import
@ -4384,8 +4415,8 @@ type ContextCreator from to
type RequestedData
= RequestedData
{ metadata : Bool
, moduleNameLookupTable : Bool
{ moduleNameLookupTable : Bool
, sourceCodeExtractor : Bool
}
@ -4409,8 +4440,8 @@ initContextCreator fromProjectToModule =
ContextCreator
(always fromProjectToModule)
(RequestedData
{ metadata = False
, moduleNameLookupTable = False
{ moduleNameLookupTable = False
, sourceCodeExtractor = False
}
)
@ -4437,10 +4468,10 @@ applyContextCreator data (ContextCreator fn _) from =
-}
withMetadata : ContextCreator Metadata (from -> to) -> ContextCreator from to
withMetadata (ContextCreator fn (RequestedData requested)) =
withMetadata (ContextCreator fn requestedData) =
ContextCreator
(\data -> fn data data.metadata)
(RequestedData { requested | metadata = True })
requestedData
{-| Requests the module name lookup table for the types and functions inside a module.
@ -4463,13 +4494,13 @@ type or value comes from.
rule : Rule
rule =
Rule.newModuleRuleSchemaUsingContextCreator "NoHtmlButton" contextCreator
Rule.newModuleRuleSchemaUsingContextCreator "NoHtmlButton" initialContext
|> Rule.withExpressionEnterVisitor expressionVisitor
|> Rule.fromModuleRuleSchema
|> Rule.ignoreErrorsForFiles [ "src/Colors.elm" ]
contextCreator : Rule.ContextCreator () Context
contextCreator =
initialContext : Rule.ContextCreator () Context
initialContext =
Rule.initContextCreator
(\lookupTable () -> { lookupTable = lookupTable })
|> Rule.withModuleNameLookupTable
@ -4517,16 +4548,49 @@ withModuleNameLookupTable (ContextCreator fn (RequestedData requested)) =
-}
withModuleKey : ContextCreator ModuleKey (from -> to) -> ContextCreator from to
withModuleKey (ContextCreator fn (RequestedData requested)) =
withModuleKey (ContextCreator fn requestedData) =
ContextCreator
(\data -> fn data data.moduleKey)
(RequestedData { requested | metadata = True })
requestedData
{-| Requests access to a function that gives you the source code at a given range.
rule : Rule
rule =
Rule.newModuleRuleSchemaUsingContextCreator YourRuleName initialContext
|> Rule.withExpressionEnterVisitor expressionVisitor
|> Rule.fromModuleRuleSchema
type alias Context =
{ extractSourceCode : Range -> String
}
initialContext : Rule.ContextCreator () Context
initialContext =
Rule.initContextCreator
(\extractSourceCode () -> { extractSourceCode = extractSourceCode })
|> Rule.withSourceCodeExtractor
The motivation for this capability was for allowing to provide higher-quality fixes, especially where you'd need to **move** or **copy**
code from one place to another (example: [when switching the branches of an if expression](https://github.com/jfmengels/elm-review/blob/master/tests/NoNegationInIfCondition.elm)).
I discourage using this functionality to explore the source code, as the different visitor functions make for a nicer
experience.
-}
withSourceCodeExtractor : ContextCreator (Range -> String) (from -> to) -> ContextCreator from to
withSourceCodeExtractor (ContextCreator fn (RequestedData requested)) =
ContextCreator
(\data -> fn data data.extractSourceCode)
(RequestedData { requested | sourceCodeExtractor = True })
type alias AvailableData =
{ metadata : Metadata
, moduleKey : ModuleKey
, moduleNameLookupTable : ModuleNameLookupTable
, extractSourceCode : Range -> String
}

119
tests/NoNegationInIfCondition.elm Normal file
View File

@ -0,0 +1,119 @@
module NoNegationInIfCondition 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.Fix as Fix
import Review.Rule as Rule exposing (Rule)
{-| Reports when negations are used in
config =
[ NoNegationInIfCondition.rule
]
## Fail
a =
if not condition then
1
else
2
## Success
a =
if condition then
1
else
2
## When (not) to enable this rule
This rule is not meant to be published. It is primarily meant to be a test rule for `withSourceCodeExtractor` and
an example for its usage.
I don't think it's a good rule to enforce, and I personally like to put the "happy" path/branch first,
instead of the non-negated one. As such, I have left this rule incomplete meaning there are more cases that should be
handled for this rule to do what it is meant to do.
-}
rule : Rule
rule =
Rule.newModuleRuleSchemaUsingContextCreator "NoNegationInIfCondition" initialContext
|> Rule.withExpressionEnterVisitor 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 {}), Context )
expressionVisitor node context =
case Node.value node of
Expression.IfBlock condition thenBranch elseBranch ->
case Node.value condition of
Expression.Application ((Node notRange (Expression.FunctionOrValue [] "not")) :: _) ->
if isIfExpr elseBranch then
( [], context )
else
let
elseString : String
elseString =
context.extractSourceCode { start = (Node.range thenBranch).end, end = (Node.range elseBranch).start }
in
( [ Rule.errorWithFix
{ message = "Don't use if expressions with negated conditions"
, details = [ "We at fruits.com think that if expressions are more readable when the condition is not negated." ]
}
notRange
[ Fix.removeRange notRange
, Fix.removeRange { start = (Node.range thenBranch).start, end = (Node.range elseBranch).start }
, Fix.insertAt
(Node.range elseBranch).end
(elseString ++ context.extractSourceCode (Node.range thenBranch))
]
]
, context
)
_ ->
( [], context )
_ ->
( [], context )
isIfExpr : Node Expression -> Bool
isIfExpr node =
case Node.value node of
Expression.ParenthesizedExpression expr ->
isIfExpr expr
Expression.IfBlock _ _ _ ->
True
_ ->
False

58
tests/NoNegationInIfConditionTest.elm Normal file
View File

@ -0,0 +1,58 @@
module NoNegationInIfConditionTest exposing (all)
import NoNegationInIfCondition exposing (rule)
import Review.Test
import Test exposing (Test, describe, test)
all : Test
all =
describe "NoNegationInIfCondition"
[ test "should not report if condition without a not call" <|
\() ->
"""module A exposing (..)
a = if condition then 1 else 2
"""
|> Review.Test.run rule
|> Review.Test.expectNoErrors
, test "should report if condition without a not call" <|
\() ->
"""module A exposing (..)
a = if not condition then 1 else 2
"""
|> Review.Test.run rule
|> Review.Test.expectErrors
[ Review.Test.error
{ message = "Don't use if expressions with negated conditions"
, details = [ "We at fruits.com think that if expressions are more readable when the condition is not negated." ]
, under = "not"
}
|> Review.Test.whenFixed """module A exposing (..)
a = if condition then 2 else 1
"""
]
, test "should report multi-line if condition without a not call" <|
\() ->
"""module A exposing (..)
a =
if not condition then
1
else
2
"""
|> Review.Test.run rule
|> Review.Test.expectErrors
[ Review.Test.error
{ message = "Don't use if expressions with negated conditions"
, details = [ "We at fruits.com think that if expressions are more readable when the condition is not negated." ]
, under = "not"
}
|> Review.Test.whenFixed """module A exposing (..)
a =
if condition then
2
else
1
"""
]
]

121
tests/NoSimpleLetBody.elm
View File

@ -79,13 +79,28 @@ elm-review --template jfmengels/elm-review-code-style/example --rules NoSimpleLe
-}
rule : Rule
rule =
Rule.newModuleRuleSchema "NoSimpleLetBody" ()
|> Rule.withSimpleExpressionVisitor expressionVisitor
Rule.newModuleRuleSchemaUsingContextCreator "NoSimpleLetBody" initialContext
|> Rule.withExpressionEnterVisitor (\node context -> ( expressionVisitor node context, context ))
|> Rule.fromModuleRuleSchema
expressionVisitor : Node Expression -> List (Rule.Error {})
expressionVisitor node =
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 } =
case Node.value node of
Expression.LetExpression { declarations, expression } ->
case Node.value expression of
@ -95,7 +110,7 @@ expressionVisitor node =
{ previousEnd : Maybe Location
, lastEnd : Maybe Location
, last : Maybe { name : String, declarationRange : Range, expressionRange : Range }
, foundDeclaredWithName : Bool
, foundDeclaredWithName : Maybe { declarationRange : Range, expressionRange : Range, takesArguments : Bool }
}
declarationData =
List.foldl
@ -119,7 +134,16 @@ expressionVisitor node =
else
Nothing
, foundDeclaredWithName = foundDeclaredWithName || Node.value functionDeclaration.name == name
, 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
}
Expression.LetDestructuring _ _ ->
@ -132,50 +156,63 @@ expressionVisitor node =
{ previousEnd = Nothing
, lastEnd = Nothing
, last = Nothing
, foundDeclaredWithName = False
, foundDeclaredWithName = Nothing
}
declarations
in
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 }
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)
]
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
[]
else
[]
Nothing ->
if not takesArguments then
[ Fix.removeRange declarationRange
, Fix.replaceRangeBy (Node.range expression) (extractSourceCode expressionRange)
]
Nothing ->
[]
)
]
else
[]
)
]
else
[]
Nothing ->
[]
_ ->
[]

10
tests/NoSimpleLetBodyTest.elm
View File

@ -74,6 +74,11 @@ 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)" <|
\() ->
@ -94,6 +99,11 @@ 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" <|
\() ->