From bbf67f3ee5a9817dd7bb4a1a3515676eeca568bc Mon Sep 17 00:00:00 2001 From: Aaron VonderHaar Date: Thu, 10 Dec 2020 12:50:12 -0800 Subject: [PATCH 1/9] WIP: ExpectEnsurePairsMatch works for trivial test case --- .gitignore | 1 + package.json | 1 + review/elm.json | 35 +++++++++++ review/src/ExpectEnsurePairsMatch.elm | 70 +++++++++++++++++++++ review/src/ReviewConfig.elm | 21 +++++++ review/tests/ExpectEnsurePairsMatchTest.elm | 36 +++++++++++ 6 files changed, 164 insertions(+) create mode 100644 review/elm.json create mode 100644 review/src/ExpectEnsurePairsMatch.elm create mode 100644 review/src/ReviewConfig.elm create mode 100644 review/tests/ExpectEnsurePairsMatchTest.elm diff --git a/.gitignore b/.gitignore index 75d3096..5225763 100644 --- a/.gitignore +++ b/.gitignore @@ -3,3 +3,4 @@ /docs.json /package-lock.json /website/node_modules/ +/review/elm-stuff/ diff --git a/package.json b/package.json index d2f8b73..4b2bb25 100644 --- a/package.json +++ b/package.json @@ -6,6 +6,7 @@ "elm": "^0.19.1-3", "elm-doc-preview": "^3.0.4", "elm-format": "0.8.3", + "elm-review": "^2.3.3", "elm-test": "^0.19.1", "vuepress": "^1.3.1" }, diff --git a/review/elm.json b/review/elm.json new file mode 100644 index 0000000..dd6745a --- /dev/null +++ b/review/elm.json @@ -0,0 +1,35 @@ +{ + "type": "application", + "source-directories": [ + "src" + ], + "elm-version": "0.19.1", + "dependencies": { + "direct": { + "elm/core": "1.0.5", + "elm/json": "1.1.3", + "elm/project-metadata-utils": "1.0.1", + "jfmengels/elm-review": "2.3.8", + "stil4m/elm-syntax": "7.1.3" + }, + "indirect": { + "elm/html": "1.0.0", + "elm/parser": "1.1.0", + "elm/random": "1.0.0", + "elm/time": "1.0.0", + "elm/url": "1.0.0", + "elm/virtual-dom": "1.0.2", + "elm-community/json-extra": "4.3.0", + "elm-community/list-extra": "8.2.4", + "rtfeldman/elm-hex": "1.0.0", + "rtfeldman/elm-iso8601-date-strings": "1.1.3", + "stil4m/structured-writer": "1.0.3" + } + }, + "test-dependencies": { + "direct": { + "elm-explorations/test": "1.2.2" + }, + "indirect": {} + } +} \ No newline at end of file diff --git a/review/src/ExpectEnsurePairsMatch.elm b/review/src/ExpectEnsurePairsMatch.elm new file mode 100644 index 0000000..4992d55 --- /dev/null +++ b/review/src/ExpectEnsurePairsMatch.elm @@ -0,0 +1,70 @@ +module ExpectEnsurePairsMatch exposing (rule) + +{-| + +@docs rule + +-} + +import Elm.Syntax.Declaration as Declaration exposing (Declaration) +import Elm.Syntax.Node as Node exposing (Node) +import Elm.Syntax.TypeAnnotation exposing (TypeAnnotation(..)) +import Review.Rule as Rule exposing (Rule) + + +{-| Makes sure that `expect*`/`ensure*` function pairs are consistent. +-} +rule : Rule +rule = + Rule.newModuleRuleSchema "ExpectEnsurePairsMatch" initContext + |> Rule.withDeclarationEnterVisitor visitor + |> Rule.fromModuleRuleSchema + + +type alias Context = + {} + + +initContext : Context +initContext = + {} + + +visitor : Node Declaration -> Context -> ( List (Rule.Error {}), Context ) +visitor node context = + ( case Node.value node of + Declaration.FunctionDeclaration function -> + let + functionName = + function.declaration + |> Node.value + |> .name + |> Node.value + in + -- TODO: only check the prefix + if functionName == "ensureSomething" then + case Maybe.map (Node.value << .typeAnnotation << Node.value) function.signature of + Just (FunctionTypeAnnotation left right) -> + -- TODO: work correctly when there's more than one initial argument + [ Rule.error + { message = "ensureSomething should take the same arguments as expectSomething" + , details = + [ "Assuming the type annotation for expectSomething is correct, the type annotation for ensureSomething should be:" + , "String -> ProgramTest msg model effect -> ProgramTest msg model effect" + ] + } + (Node.range left) + ] + + _ -> + -- TODO: report that ensure* must have type annotation + -- TODO: report that ensure* must be a function + [] + + else + [] + + _ -> + [] + , context + ) diff --git a/review/src/ReviewConfig.elm b/review/src/ReviewConfig.elm new file mode 100644 index 0000000..bbf7c17 --- /dev/null +++ b/review/src/ReviewConfig.elm @@ -0,0 +1,21 @@ +module ReviewConfig exposing (config) + +{-| Do not rename the ReviewConfig module or the config function, because +`elm-review` will look for these. + +To add packages that contain rules, add them to this review project using + + `elm install author/packagename` + +when inside the directory containing this file. + +-} + +import ExpectEnsurePairsMatch +import Review.Rule exposing (Rule) + + +config : List Rule +config = + [ ExpectEnsurePairsMatch.rule + ] diff --git a/review/tests/ExpectEnsurePairsMatchTest.elm b/review/tests/ExpectEnsurePairsMatchTest.elm new file mode 100644 index 0000000..9c017b0 --- /dev/null +++ b/review/tests/ExpectEnsurePairsMatchTest.elm @@ -0,0 +1,36 @@ +module ExpectEnsurePairsMatchTest exposing (all) + +import ExpectEnsurePairsMatch exposing (rule) +import Review.Test +import Test exposing (Test, describe, test) + + +all : Test +all = + describe "ExpectEnsurePairsMatch" + [ test "expect/ensure pair must have the same arguments" <| + \() -> + String.join "\n" + [ "module ProgramTest exposing (expectSomething, ensureSomething)" + , "import Expect exposing (Expectation)" + , "" + , "type alias ProgramTest msg model effect = ()" + , "" + , "expectSomething : String -> ProgramTest msg model effect -> Expectation" + , "expectSomething = Debug.todo \"expectSomething\"" + , "" + , "ensureSomething : Int -> ProgramTest msg model effect -> ProgramTest msg model effect" + , "ensureSomething = Debug.todo \"expectSomething\"" + ] + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "ensureSomething should take the same arguments as expectSomething" + , details = + [ "Assuming the type annotation for expectSomething is correct, the type annotation for ensureSomething should be:" + , "String -> ProgramTest msg model effect -> ProgramTest msg model effect" + ] + , under = "Int" + } + ] + ] From 73113065f97c831a439dc6b53df1a0612b757825 Mon Sep 17 00:00:00 2001 From: Aaron VonderHaar Date: Thu, 10 Dec 2020 13:37:31 -0800 Subject: [PATCH 2/9] WIP: ExpectEnsurePairsMatch checks the first argument against the rule context --- review/src/ElmSyntaxHelper.elm | 45 ++++++++++ review/src/ExpectEnsurePairsMatch.elm | 95 +++++++++++++++------ review/tests/ExpectEnsurePairsMatchTest.elm | 16 ++++ 3 files changed, 128 insertions(+), 28 deletions(-) create mode 100644 review/src/ElmSyntaxHelper.elm diff --git a/review/src/ElmSyntaxHelper.elm b/review/src/ElmSyntaxHelper.elm new file mode 100644 index 0000000..dc0c7f5 --- /dev/null +++ b/review/src/ElmSyntaxHelper.elm @@ -0,0 +1,45 @@ +module ElmSyntaxHelper exposing (removeTypeAnnotationRange) + +{-| from +-} + +import Elm.Syntax.Node as Node exposing (Node(..)) +import Elm.Syntax.Range as Range +import Elm.Syntax.TypeAnnotation exposing (TypeAnnotation(..)) + + +removeTypeAnnotationRange : Node TypeAnnotation -> Node TypeAnnotation +removeTypeAnnotationRange (Node _ value) = + Node Range.emptyRange + (case value of + FunctionTypeAnnotation input output -> + FunctionTypeAnnotation (removeTypeAnnotationRange input) (removeTypeAnnotationRange output) + + Typed (Node _ nameNode) params -> + Typed (Node Range.emptyRange nameNode) (List.map removeTypeAnnotationRange params) + + GenericType string -> + GenericType string + + Unit -> + Unit + + Tupled nodes -> + Tupled (List.map removeTypeAnnotationRange nodes) + + Record recordDefinition -> + Record + (List.map + (Node.value + >> (\( Node _ field, Node _ type_ ) -> + Node Range.emptyRange ( Node Range.emptyRange field, Node Range.emptyRange type_ ) + ) + ) + recordDefinition + ) + + GenericRecord (Node _ var) (Node _ recordDefinition) -> + GenericRecord + (Node Range.emptyRange var) + (Node Range.emptyRange recordDefinition) + ) diff --git a/review/src/ExpectEnsurePairsMatch.elm b/review/src/ExpectEnsurePairsMatch.elm index 4992d55..6754e91 100644 --- a/review/src/ExpectEnsurePairsMatch.elm +++ b/review/src/ExpectEnsurePairsMatch.elm @@ -6,9 +6,12 @@ module ExpectEnsurePairsMatch exposing (rule) -} +import Dict exposing (Dict) import Elm.Syntax.Declaration as Declaration exposing (Declaration) -import Elm.Syntax.Node as Node exposing (Node) +import Elm.Syntax.Node as Node exposing (Node(..)) +import Elm.Syntax.Range exposing (emptyRange) import Elm.Syntax.TypeAnnotation exposing (TypeAnnotation(..)) +import ElmSyntaxHelper exposing (removeTypeAnnotationRange) import Review.Rule as Rule exposing (Rule) @@ -17,21 +20,40 @@ import Review.Rule as Rule exposing (Rule) rule : Rule rule = Rule.newModuleRuleSchema "ExpectEnsurePairsMatch" initContext - |> Rule.withDeclarationEnterVisitor visitor + |> Rule.withDeclarationListVisitor collectExpectTypes + |> Rule.withDeclarationEnterVisitor validateEnsureTypes |> Rule.fromModuleRuleSchema type alias Context = - {} + { expectFunctionArguments : Dict String (List TypeAnnotation) + } initContext : Context initContext = - {} + { expectFunctionArguments = Dict.empty + } -visitor : Node Declaration -> Context -> ( List (Rule.Error {}), Context ) -visitor node context = +collectExpectTypes : List (Node Declaration) -> Context -> ( List never, Context ) +collectExpectTypes declarations context = + ( [] + , { context + | expectFunctionArguments = + -- TODO: build form the actual declarations + Dict.fromList + [ ( "Something" + , [ Typed (Node emptyRange ( [], "String" )) [] + ] + ) + ] + } + ) + + +validateEnsureTypes : Node Declaration -> Context -> ( List (Rule.Error {}), Context ) +validateEnsureTypes node context = ( case Node.value node of Declaration.FunctionDeclaration function -> let @@ -40,29 +62,46 @@ visitor node context = |> Node.value |> .name |> Node.value + + ensureName = + if String.startsWith "ensure" functionName then + Just (String.dropLeft 6 functionName) + + else + Nothing in - -- TODO: only check the prefix - if functionName == "ensureSomething" then - case Maybe.map (Node.value << .typeAnnotation << Node.value) function.signature of - Just (FunctionTypeAnnotation left right) -> - -- TODO: work correctly when there's more than one initial argument - [ Rule.error - { message = "ensureSomething should take the same arguments as expectSomething" - , details = - [ "Assuming the type annotation for expectSomething is correct, the type annotation for ensureSomething should be:" - , "String -> ProgramTest msg model effect -> ProgramTest msg model effect" - ] - } - (Node.range left) - ] - - _ -> - -- TODO: report that ensure* must have type annotation - -- TODO: report that ensure* must be a function - [] - - else - [] + case ensureName of + Just name -> + case Maybe.map (Node.value << .typeAnnotation << Node.value) function.signature of + Just (FunctionTypeAnnotation left right) -> + case Dict.get name context.expectFunctionArguments of + Just [ first ] -> + if (Node.value <| removeTypeAnnotationRange left) == first then + [] + + else + [ Rule.error + { message = "ensureSomething should take the same arguments as expectSomething" + , details = + [ "Assuming the type annotation for expectSomething is correct, the type annotation for ensureSomething should be:" + , "String -> ProgramTest msg model effect -> ProgramTest msg model effect" + ] + } + (Node.range left) + ] + + _ -> + -- TODO: work correctly when there's more than one initial argument + -- TODO: report when the corresponding expect function doesn't exist + [] + + _ -> + -- TODO: report that ensure* must have type annotation + -- TODO: report that ensure* must be a function + [] + + _ -> + [] _ -> [] diff --git a/review/tests/ExpectEnsurePairsMatchTest.elm b/review/tests/ExpectEnsurePairsMatchTest.elm index 9c017b0..93e7f11 100644 --- a/review/tests/ExpectEnsurePairsMatchTest.elm +++ b/review/tests/ExpectEnsurePairsMatchTest.elm @@ -33,4 +33,20 @@ all = , under = "Int" } ] + , test "expect/ensure pair with correct arguments" <| + \() -> + String.join "\n" + [ "module ProgramTest exposing (expectSomething, ensureSomething)" + , "import Expect exposing (Expectation)" + , "" + , "type alias ProgramTest msg model effect = ()" + , "" + , "expectSomething : String -> ProgramTest msg model effect -> Expectation" + , "expectSomething = Debug.todo \"expectSomething\"" + , "" + , "ensureSomething : String -> ProgramTest msg model effect -> ProgramTest msg model effect" + , "ensureSomething = Debug.todo \"expectSomething\"" + ] + |> Review.Test.run rule + |> Review.Test.expectNoErrors ] From 78ce1595bc8f62c52a2c58d76fa410dbdb4c4b98 Mon Sep 17 00:00:00 2001 From: Aaron VonderHaar Date: Thu, 10 Dec 2020 13:53:58 -0800 Subject: [PATCH 3/9] Add jfmengels/elm-review-debug checks --- package.json | 3 ++- review/elm.json | 1 + review/src/ReviewConfig.elm | 7 ++++++- 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/package.json b/package.json index 4b2bb25..1303fbd 100644 --- a/package.json +++ b/package.json @@ -11,7 +11,8 @@ "vuepress": "^1.3.1" }, "scripts": { - "test": "elm-test && npm run-script build-examples && elm make --docs docs.json && elm-format --validate . && elm diff", + "test": "elm-test && npm run-script build-examples && elm make --docs docs.json && npm run-script check && (cd review && elm-test) && elm diff", + "check": "elm-format --validate . && elm-review", "build-examples": "(cd examples && elm make src/*.elm --output=/dev/null && elm-test)", "run-examples": "(cd examples && elm reactor --port 8002)", "run-examples-backend": "(cd examples && PORT=8003 ./node_modules/.bin/nodemon server.js)", diff --git a/review/elm.json b/review/elm.json index dd6745a..04b1734 100644 --- a/review/elm.json +++ b/review/elm.json @@ -10,6 +10,7 @@ "elm/json": "1.1.3", "elm/project-metadata-utils": "1.0.1", "jfmengels/elm-review": "2.3.8", + "jfmengels/elm-review-debug": "1.0.3", "stil4m/elm-syntax": "7.1.3" }, "indirect": { diff --git a/review/src/ReviewConfig.elm b/review/src/ReviewConfig.elm index bbf7c17..307d515 100644 --- a/review/src/ReviewConfig.elm +++ b/review/src/ReviewConfig.elm @@ -12,10 +12,15 @@ when inside the directory containing this file. -} import ExpectEnsurePairsMatch -import Review.Rule exposing (Rule) +import NoDebug.Log +import NoDebug.TodoOrToString +import Review.Rule as Rule exposing (Rule) config : List Rule config = [ ExpectEnsurePairsMatch.rule + , NoDebug.Log.rule + , NoDebug.TodoOrToString.rule + |> Rule.ignoreErrorsForDirectories [ "tests/" ] ] From e54c433c0ddcf237da1e30431bfe98b7c00f749c Mon Sep 17 00:00:00 2001 From: Aaron VonderHaar Date: Thu, 10 Dec 2020 14:17:44 -0800 Subject: [PATCH 4/9] WIP: ExpectEnsurePairsMatch collect real function names from the module --- review/src/ExpectEnsurePairsMatch.elm | 46 +++++++++++++++------------ 1 file changed, 26 insertions(+), 20 deletions(-) diff --git a/review/src/ExpectEnsurePairsMatch.elm b/review/src/ExpectEnsurePairsMatch.elm index 6754e91..63c108e 100644 --- a/review/src/ExpectEnsurePairsMatch.elm +++ b/review/src/ExpectEnsurePairsMatch.elm @@ -41,20 +41,15 @@ collectExpectTypes declarations context = ( [] , { context | expectFunctionArguments = - -- TODO: build form the actual declarations - Dict.fromList - [ ( "Something" - , [ Typed (Node emptyRange ( [], "String" )) [] - ] - ) - ] + List.filterMap (getNamedFunctionType "expect") declarations + |> Dict.fromList } ) -validateEnsureTypes : Node Declaration -> Context -> ( List (Rule.Error {}), Context ) -validateEnsureTypes node context = - ( case Node.value node of +getNamedFunctionType : String -> Node Declaration -> Maybe ( String, List TypeAnnotation ) +getNamedFunctionType prefix declaration = + case Node.value declaration of Declaration.FunctionDeclaration function -> let functionName = @@ -62,16 +57,27 @@ validateEnsureTypes node context = |> Node.value |> .name |> Node.value + in + if String.startsWith prefix functionName then + Just + ( String.dropLeft (String.length prefix) functionName + , -- TODO: build form the actual declarations + [ Typed (Node emptyRange ( [], "String" )) [] ] + ) - ensureName = - if String.startsWith "ensure" functionName then - Just (String.dropLeft 6 functionName) + else + Nothing - else - Nothing - in - case ensureName of - Just name -> + _ -> + Nothing + + +validateEnsureTypes : Node Declaration -> Context -> ( List (Rule.Error {}), Context ) +validateEnsureTypes node context = + ( case Node.value node of + Declaration.FunctionDeclaration function -> + case getNamedFunctionType "ensure" node of + Just ( name, actualArgs ) -> case Maybe.map (Node.value << .typeAnnotation << Node.value) function.signature of Just (FunctionTypeAnnotation left right) -> case Dict.get name context.expectFunctionArguments of @@ -81,9 +87,9 @@ validateEnsureTypes node context = else [ Rule.error - { message = "ensureSomething should take the same arguments as expectSomething" + { message = "ensure" ++ name ++ " should take the same arguments as expect" ++ name , details = - [ "Assuming the type annotation for expectSomething is correct, the type annotation for ensureSomething should be:" + [ "Assuming the type annotation for expect" ++ name ++ " is correct, the type annotation for ensure" ++ name ++ " should be:" , "String -> ProgramTest msg model effect -> ProgramTest msg model effect" ] } From 91e943fecea9694f4c4330b85f6a21a6720b8727 Mon Sep 17 00:00:00 2001 From: Aaron VonderHaar Date: Thu, 10 Dec 2020 14:40:30 -0800 Subject: [PATCH 5/9] WIP: ExpectEnsurePairsMatch collect real function arguments from the module --- review/src/ExpectEnsurePairsMatch.elm | 56 ++++++++++++++++++++++----- 1 file changed, 46 insertions(+), 10 deletions(-) diff --git a/review/src/ExpectEnsurePairsMatch.elm b/review/src/ExpectEnsurePairsMatch.elm index 63c108e..ec3c48f 100644 --- a/review/src/ExpectEnsurePairsMatch.elm +++ b/review/src/ExpectEnsurePairsMatch.elm @@ -9,7 +9,6 @@ module ExpectEnsurePairsMatch exposing (rule) import Dict exposing (Dict) import Elm.Syntax.Declaration as Declaration exposing (Declaration) import Elm.Syntax.Node as Node exposing (Node(..)) -import Elm.Syntax.Range exposing (emptyRange) import Elm.Syntax.TypeAnnotation exposing (TypeAnnotation(..)) import ElmSyntaxHelper exposing (removeTypeAnnotationRange) import Review.Rule as Rule exposing (Rule) @@ -49,6 +48,32 @@ collectExpectTypes declarations context = getNamedFunctionType : String -> Node Declaration -> Maybe ( String, List TypeAnnotation ) getNamedFunctionType prefix declaration = + case getFunctionType declaration of + Ok ( functionName, args ) -> + if String.startsWith prefix functionName then + Just + ( String.dropLeft (String.length prefix) functionName + , List.take 1 args + -- TODO: validate that return value is Expectation + -- TODO: validate that last arg is ProgramTest msg model effect + -- TODO: work correctly when there is more than one initial arg + ) + + else + Nothing + + _ -> + -- TODO: report if there was no type annotation + Nothing + + +type FunctionParseError + = NotAFunction + | NoTypeAnnotation + + +getFunctionType : Node Declaration -> Result FunctionParseError ( String, List TypeAnnotation ) +getFunctionType declaration = case Node.value declaration of Declaration.FunctionDeclaration function -> let @@ -58,18 +83,29 @@ getNamedFunctionType prefix declaration = |> .name |> Node.value in - if String.startsWith prefix functionName then - Just - ( String.dropLeft (String.length prefix) functionName - , -- TODO: build form the actual declarations - [ Typed (Node emptyRange ( [], "String" )) [] ] - ) + case Maybe.map (.typeAnnotation << Node.value) function.signature of + Just annotation -> + Ok + ( functionName + , flattenFunctionType annotation + ) - else - Nothing + _ -> + Err NoTypeAnnotation _ -> - Nothing + Err NotAFunction + + +flattenFunctionType : Node TypeAnnotation -> List TypeAnnotation +flattenFunctionType typeAnnotation = + case Node.value <| removeTypeAnnotationRange typeAnnotation of + FunctionTypeAnnotation left right -> + -- TODO: recurse into right + [ Node.value left, Node.value right ] + + other -> + [ other ] validateEnsureTypes : Node Declaration -> Context -> ( List (Rule.Error {}), Context ) From 0b8b2e8a024b4aa5e33292c9db3728c4ab03d2ef Mon Sep 17 00:00:00 2001 From: Aaron VonderHaar Date: Fri, 11 Dec 2020 19:06:16 -0800 Subject: [PATCH 6/9] WIP: ExpectEnsurePairsMatch ensure function must have type annotation --- review/src/ExpectEnsurePairsMatch.elm | 70 +++++++++++++-------- review/tests/ExpectEnsurePairsMatchTest.elm | 25 ++++++++ 2 files changed, 69 insertions(+), 26 deletions(-) diff --git a/review/src/ExpectEnsurePairsMatch.elm b/review/src/ExpectEnsurePairsMatch.elm index ec3c48f..4831da6 100644 --- a/review/src/ExpectEnsurePairsMatch.elm +++ b/review/src/ExpectEnsurePairsMatch.elm @@ -40,39 +40,53 @@ collectExpectTypes declarations context = ( [] , { context | expectFunctionArguments = - List.filterMap (getNamedFunctionType "expect") declarations + -- TODO: report when expect functions aren't valid + List.filterMap + (\decl -> + case getNamedFunctionType "expect" decl of + Just ( name, Ok args ) -> + Just ( name, args ) + + _ -> + Nothing + ) + declarations |> Dict.fromList } ) -getNamedFunctionType : String -> Node Declaration -> Maybe ( String, List TypeAnnotation ) +getNamedFunctionType : String -> Node Declaration -> Maybe ( String, Result FunctionParseError (List TypeAnnotation) ) getNamedFunctionType prefix declaration = case getFunctionType declaration of - Ok ( functionName, args ) -> - if String.startsWith prefix functionName then + Just ( functionName, annotation ) -> + if String.startsWith prefix (Node.value functionName) then Just - ( String.dropLeft (String.length prefix) functionName - , List.take 1 args - -- TODO: validate that return value is Expectation - -- TODO: validate that last arg is ProgramTest msg model effect - -- TODO: work correctly when there is more than one initial arg + ( String.dropLeft (String.length prefix) (Node.value functionName) + , case annotation of + Just args -> + -- TODO: validate that return value is Expectation + -- TODO: validate that last arg is ProgramTest msg model effect + -- TODO: work correctly when there is more than one initial arg + Ok (List.take 1 args) + + Nothing -> + Err (NoTypeAnnotation functionName) ) else Nothing - _ -> - -- TODO: report if there was no type annotation + Nothing -> Nothing type FunctionParseError = NotAFunction - | NoTypeAnnotation + | NoTypeAnnotation (Node String) -getFunctionType : Node Declaration -> Result FunctionParseError ( String, List TypeAnnotation ) +getFunctionType : Node Declaration -> Maybe ( Node String, Maybe (List TypeAnnotation) ) getFunctionType declaration = case Node.value declaration of Declaration.FunctionDeclaration function -> @@ -81,20 +95,14 @@ getFunctionType declaration = function.declaration |> Node.value |> .name - |> Node.value in - case Maybe.map (.typeAnnotation << Node.value) function.signature of - Just annotation -> - Ok - ( functionName - , flattenFunctionType annotation - ) - - _ -> - Err NoTypeAnnotation + Just + ( functionName + , Maybe.map (flattenFunctionType << .typeAnnotation << Node.value) function.signature + ) _ -> - Err NotAFunction + Nothing flattenFunctionType : Node TypeAnnotation -> List TypeAnnotation @@ -113,7 +121,7 @@ validateEnsureTypes node context = ( case Node.value node of Declaration.FunctionDeclaration function -> case getNamedFunctionType "ensure" node of - Just ( name, actualArgs ) -> + Just ( name, Ok actualArgs ) -> case Maybe.map (Node.value << .typeAnnotation << Node.value) function.signature of Just (FunctionTypeAnnotation left right) -> case Dict.get name context.expectFunctionArguments of @@ -138,10 +146,20 @@ validateEnsureTypes node context = [] _ -> - -- TODO: report that ensure* must have type annotation -- TODO: report that ensure* must be a function [] + Just ( name, Err (NoTypeAnnotation functionName) ) -> + [ Rule.error + { message = Node.value functionName ++ " must have a type annotation" + , details = + [ "Assuming the type annotation for expect" ++ name ++ " is correct, the type annotation for ensure" ++ name ++ " should be:" + , "String -> ProgramTest msg model effect -> ProgramTest msg model effect" + ] + } + (Node.range functionName) + ] + _ -> [] diff --git a/review/tests/ExpectEnsurePairsMatchTest.elm b/review/tests/ExpectEnsurePairsMatchTest.elm index 93e7f11..14de11a 100644 --- a/review/tests/ExpectEnsurePairsMatchTest.elm +++ b/review/tests/ExpectEnsurePairsMatchTest.elm @@ -49,4 +49,29 @@ all = ] |> Review.Test.run rule |> Review.Test.expectNoErrors + , test "ensure function must have type annotation" <| + \() -> + String.join "\n" + [ "module ProgramTest exposing (expectSomething, ensureSomething)" + , "import Expect exposing (Expectation)" + , "" + , "type alias ProgramTest msg model effect = ()" + , "" + , "expectSomething : String -> ProgramTest msg model effect -> Expectation" + , "expectSomething = Debug.todo \"expectSomething\"" + , "" + , "ensureSomething = Debug.todo \"expectSomething\"" + ] + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "ensureSomething must have a type annotation" + , details = + [ "Assuming the type annotation for expectSomething is correct, the type annotation for ensureSomething should be:" + , "String -> ProgramTest msg model effect -> ProgramTest msg model effect" + ] + , under = "ensureSomething" + } + |> Review.Test.atExactly { start = { row = 9, column = 1 }, end = { row = 9, column = 16 } } + ] ] From 226c18e2150e21ee0f0331c7750e0633a1e0f395 Mon Sep 17 00:00:00 2001 From: Aaron VonderHaar Date: Fri, 11 Dec 2020 19:18:19 -0800 Subject: [PATCH 7/9] WIP: ExpectEnsurePairsMatch remove redundant case expressions --- review/src/ExpectEnsurePairsMatch.elm | 88 ++++++++++++--------------- 1 file changed, 39 insertions(+), 49 deletions(-) diff --git a/review/src/ExpectEnsurePairsMatch.elm b/review/src/ExpectEnsurePairsMatch.elm index 4831da6..2730dde 100644 --- a/review/src/ExpectEnsurePairsMatch.elm +++ b/review/src/ExpectEnsurePairsMatch.elm @@ -25,7 +25,7 @@ rule = type alias Context = - { expectFunctionArguments : Dict String (List TypeAnnotation) + { expectFunctionArguments : Dict String (List (Node TypeAnnotation)) } @@ -56,7 +56,7 @@ collectExpectTypes declarations context = ) -getNamedFunctionType : String -> Node Declaration -> Maybe ( String, Result FunctionParseError (List TypeAnnotation) ) +getNamedFunctionType : String -> Node Declaration -> Maybe ( String, Result FunctionParseError (List (Node TypeAnnotation)) ) getNamedFunctionType prefix declaration = case getFunctionType declaration of Just ( functionName, annotation ) -> @@ -86,7 +86,7 @@ type FunctionParseError | NoTypeAnnotation (Node String) -getFunctionType : Node Declaration -> Maybe ( Node String, Maybe (List TypeAnnotation) ) +getFunctionType : Node Declaration -> Maybe ( Node String, Maybe (List (Node TypeAnnotation)) ) getFunctionType declaration = case Node.value declaration of Declaration.FunctionDeclaration function -> @@ -105,64 +105,54 @@ getFunctionType declaration = Nothing -flattenFunctionType : Node TypeAnnotation -> List TypeAnnotation +flattenFunctionType : Node TypeAnnotation -> List (Node TypeAnnotation) flattenFunctionType typeAnnotation = - case Node.value <| removeTypeAnnotationRange typeAnnotation of + case Node.value typeAnnotation of FunctionTypeAnnotation left right -> -- TODO: recurse into right - [ Node.value left, Node.value right ] + [ left, right ] - other -> - [ other ] + _ -> + [ typeAnnotation ] validateEnsureTypes : Node Declaration -> Context -> ( List (Rule.Error {}), Context ) validateEnsureTypes node context = - ( case Node.value node of - Declaration.FunctionDeclaration function -> - case getNamedFunctionType "ensure" node of - Just ( name, Ok actualArgs ) -> - case Maybe.map (Node.value << .typeAnnotation << Node.value) function.signature of - Just (FunctionTypeAnnotation left right) -> - case Dict.get name context.expectFunctionArguments of - Just [ first ] -> - if (Node.value <| removeTypeAnnotationRange left) == first then - [] - - else - [ Rule.error - { message = "ensure" ++ name ++ " should take the same arguments as expect" ++ name - , details = - [ "Assuming the type annotation for expect" ++ name ++ " is correct, the type annotation for ensure" ++ name ++ " should be:" - , "String -> ProgramTest msg model effect -> ProgramTest msg model effect" - ] - } - (Node.range left) - ] - - _ -> - -- TODO: work correctly when there's more than one initial argument - -- TODO: report when the corresponding expect function doesn't exist - [] - - _ -> - -- TODO: report that ensure* must be a function - [] - - Just ( name, Err (NoTypeAnnotation functionName) ) -> - [ Rule.error - { message = Node.value functionName ++ " must have a type annotation" - , details = - [ "Assuming the type annotation for expect" ++ name ++ " is correct, the type annotation for ensure" ++ name ++ " should be:" - , "String -> ProgramTest msg model effect -> ProgramTest msg model effect" - ] - } - (Node.range functionName) - ] + ( case getNamedFunctionType "ensure" node of + Just ( name, Ok [ left ] ) -> + case Dict.get name context.expectFunctionArguments of + Just [ first ] -> + if removeTypeAnnotationRange left == removeTypeAnnotationRange first then + [] + + else + [ Rule.error + { message = "ensure" ++ name ++ " should take the same arguments as expect" ++ name + , details = + [ "Assuming the type annotation for expect" ++ name ++ " is correct, the type annotation for ensure" ++ name ++ " should be:" + , "String -> ProgramTest msg model effect -> ProgramTest msg model effect" + ] + } + (Node.range left) + ] _ -> + -- TODO: work correctly when there's more than one initial argument + -- TODO: report when the corresponding expect function doesn't exist + -- TODO: report that ensure* must be a function [] + Just ( name, Err (NoTypeAnnotation functionName) ) -> + [ Rule.error + { message = Node.value functionName ++ " must have a type annotation" + , details = + [ "Assuming the type annotation for expect" ++ name ++ " is correct, the type annotation for ensure" ++ name ++ " should be:" + , "String -> ProgramTest msg model effect -> ProgramTest msg model effect" + ] + } + (Node.range functionName) + ] + _ -> [] , context From 52f59b29ff9bb32081ef1ada1d7d1b4f62cb6ccc Mon Sep 17 00:00:00 2001 From: Aaron VonderHaar Date: Fri, 11 Dec 2020 20:21:41 -0800 Subject: [PATCH 8/9] WIP: ExpectEnsurePairsMatch argument match works when there is more than one initial argument --- review/elm.json | 1 + review/src/ElmSyntaxHelper.elm | 37 +++++++++++++++- review/src/ExpectEnsurePairsMatch.elm | 48 ++++++++++++++------- review/tests/ExpectEnsurePairsMatchTest.elm | 25 +++++++++++ 4 files changed, 93 insertions(+), 18 deletions(-) diff --git a/review/elm.json b/review/elm.json index 04b1734..fe86646 100644 --- a/review/elm.json +++ b/review/elm.json @@ -11,6 +11,7 @@ "elm/project-metadata-utils": "1.0.1", "jfmengels/elm-review": "2.3.8", "jfmengels/elm-review-debug": "1.0.3", + "mgold/elm-nonempty-list": "4.1.0", "stil4m/elm-syntax": "7.1.3" }, "indirect": { diff --git a/review/src/ElmSyntaxHelper.elm b/review/src/ElmSyntaxHelper.elm index dc0c7f5..1c34cd5 100644 --- a/review/src/ElmSyntaxHelper.elm +++ b/review/src/ElmSyntaxHelper.elm @@ -1,11 +1,11 @@ -module ElmSyntaxHelper exposing (removeTypeAnnotationRange) +module ElmSyntaxHelper exposing (removeTypeAnnotationRange, typeAnnotationToString) {-| from -} import Elm.Syntax.Node as Node exposing (Node(..)) import Elm.Syntax.Range as Range -import Elm.Syntax.TypeAnnotation exposing (TypeAnnotation(..)) +import Elm.Syntax.TypeAnnotation exposing (RecordField, TypeAnnotation(..)) removeTypeAnnotationRange : Node TypeAnnotation -> Node TypeAnnotation @@ -43,3 +43,36 @@ removeTypeAnnotationRange (Node _ value) = (Node Range.emptyRange var) (Node Range.emptyRange recordDefinition) ) + + +typeAnnotationToString : Node TypeAnnotation -> String +typeAnnotationToString node = + case Node.value node of + GenericType string -> + string + + Typed (Node _ ( [], name )) args -> + String.join " " (name :: List.map typeAnnotationToString args) + + Typed (Node _ ( moduleName, name )) args -> + String.join " " ((String.join "." moduleName ++ "." ++ name) :: List.map typeAnnotationToString args) + + Unit -> + "()" + + Tupled items -> + "( " ++ String.join ", " (List.map typeAnnotationToString items) ++ " )" + + Record fields -> + "{ " ++ String.join ", " (List.map recordFieldToString fields) ++ " }" + + GenericRecord (Node _ base) (Node _ fields) -> + "{ " ++ base ++ " | " ++ String.join ", " (List.map recordFieldToString fields) ++ " }" + + FunctionTypeAnnotation left right -> + "(" ++ typeAnnotationToString left ++ " -> " ++ typeAnnotationToString right ++ ")" + + +recordFieldToString : Node RecordField -> String +recordFieldToString (Node _ ( Node _ key, value )) = + key ++ " : " ++ typeAnnotationToString value diff --git a/review/src/ExpectEnsurePairsMatch.elm b/review/src/ExpectEnsurePairsMatch.elm index 2730dde..42dd070 100644 --- a/review/src/ExpectEnsurePairsMatch.elm +++ b/review/src/ExpectEnsurePairsMatch.elm @@ -9,8 +9,10 @@ module ExpectEnsurePairsMatch exposing (rule) import Dict exposing (Dict) import Elm.Syntax.Declaration as Declaration exposing (Declaration) import Elm.Syntax.Node as Node exposing (Node(..)) +import Elm.Syntax.Range as Range import Elm.Syntax.TypeAnnotation exposing (TypeAnnotation(..)) -import ElmSyntaxHelper exposing (removeTypeAnnotationRange) +import ElmSyntaxHelper exposing (removeTypeAnnotationRange, typeAnnotationToString) +import List.Nonempty as Nonempty exposing (Nonempty) import Review.Rule as Rule exposing (Rule) @@ -63,12 +65,14 @@ getNamedFunctionType prefix declaration = if String.startsWith prefix (Node.value functionName) then Just ( String.dropLeft (String.length prefix) (Node.value functionName) - , case annotation of - Just args -> + , case Maybe.map (List.reverse << Nonempty.toList) annotation of + Just (returnType :: programTest :: args) -> -- TODO: validate that return value is Expectation -- TODO: validate that last arg is ProgramTest msg model effect - -- TODO: work correctly when there is more than one initial arg - Ok (List.take 1 args) + Ok (List.reverse args) + + Just _ -> + Err NotEnoughArgs Nothing -> Err (NoTypeAnnotation functionName) @@ -83,10 +87,11 @@ getNamedFunctionType prefix declaration = type FunctionParseError = NotAFunction + | NotEnoughArgs | NoTypeAnnotation (Node String) -getFunctionType : Node Declaration -> Maybe ( Node String, Maybe (List (Node TypeAnnotation)) ) +getFunctionType : Node Declaration -> Maybe ( Node String, Maybe (Nonempty (Node TypeAnnotation)) ) getFunctionType declaration = case Node.value declaration of Declaration.FunctionDeclaration function -> @@ -105,24 +110,35 @@ getFunctionType declaration = Nothing -flattenFunctionType : Node TypeAnnotation -> List (Node TypeAnnotation) +flattenFunctionType : Node TypeAnnotation -> Nonempty (Node TypeAnnotation) flattenFunctionType typeAnnotation = case Node.value typeAnnotation of FunctionTypeAnnotation left right -> - -- TODO: recurse into right - [ left, right ] + Nonempty.cons left (flattenFunctionType right) _ -> - [ typeAnnotation ] + Nonempty.fromElement typeAnnotation validateEnsureTypes : Node Declaration -> Context -> ( List (Rule.Error {}), Context ) validateEnsureTypes node context = ( case getNamedFunctionType "ensure" node of - Just ( name, Ok [ left ] ) -> + Just ( name, Ok ensureArgs ) -> case Dict.get name context.expectFunctionArguments of - Just [ first ] -> - if removeTypeAnnotationRange left == removeTypeAnnotationRange first then + Just expectArgs -> + let + checkArg ensureArg expectArg = + if removeTypeAnnotationRange ensureArg == removeTypeAnnotationRange expectArg then + Nothing + + else + Just ensureArg + + mismatchedArgs = + List.map2 checkArg ensureArgs expectArgs + |> List.filterMap identity + in + if List.isEmpty mismatchedArgs then [] else @@ -130,14 +146,14 @@ validateEnsureTypes node context = { message = "ensure" ++ name ++ " should take the same arguments as expect" ++ name , details = [ "Assuming the type annotation for expect" ++ name ++ " is correct, the type annotation for ensure" ++ name ++ " should be:" - , "String -> ProgramTest msg model effect -> ProgramTest msg model effect" + , String.join " -> " (List.map typeAnnotationToString expectArgs) + ++ " -> ProgramTest msg model effect -> ProgramTest msg model effect" ] } - (Node.range left) + (Range.combine <| List.map Node.range mismatchedArgs) ] _ -> - -- TODO: work correctly when there's more than one initial argument -- TODO: report when the corresponding expect function doesn't exist -- TODO: report that ensure* must be a function [] diff --git a/review/tests/ExpectEnsurePairsMatchTest.elm b/review/tests/ExpectEnsurePairsMatchTest.elm index 14de11a..f55894c 100644 --- a/review/tests/ExpectEnsurePairsMatchTest.elm +++ b/review/tests/ExpectEnsurePairsMatchTest.elm @@ -33,6 +33,31 @@ all = , under = "Int" } ] + , test "expect/ensure pair must have the same arguments (more than one initial argument)" <| + \() -> + String.join "\n" + [ "module ProgramTest exposing (expectSomething, ensureSomething)" + , "import Expect exposing (Expectation)" + , "" + , "type alias ProgramTest msg model effect = ()" + , "" + , "expectSomething : Bool -> String -> ProgramTest msg model effect -> Expectation" + , "expectSomething = Debug.todo \"expectSomething\"" + , "" + , "ensureSomething : Bool -> Int -> ProgramTest msg model effect -> ProgramTest msg model effect" + , "ensureSomething = Debug.todo \"expectSomething\"" + ] + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "ensureSomething should take the same arguments as expectSomething" + , details = + [ "Assuming the type annotation for expectSomething is correct, the type annotation for ensureSomething should be:" + , "Bool -> String -> ProgramTest msg model effect -> ProgramTest msg model effect" + ] + , under = "Int" + } + ] , test "expect/ensure pair with correct arguments" <| \() -> String.join "\n" From d8a2eb669d09910bb0ef726abe4861d1165fd3b9 Mon Sep 17 00:00:00 2001 From: Aaron VonderHaar Date: Fri, 11 Dec 2020 20:40:58 -0800 Subject: [PATCH 9/9] ExpectEnsurePairsMatch ensure functions must have corresponding expect functions --- review/src/ExpectEnsurePairsMatch.elm | 37 ++++++++++++++------- review/tests/ExpectEnsurePairsMatchTest.elm | 23 +++++++++++++ 2 files changed, 48 insertions(+), 12 deletions(-) diff --git a/review/src/ExpectEnsurePairsMatch.elm b/review/src/ExpectEnsurePairsMatch.elm index 42dd070..5685a02 100644 --- a/review/src/ExpectEnsurePairsMatch.elm +++ b/review/src/ExpectEnsurePairsMatch.elm @@ -46,7 +46,7 @@ collectExpectTypes declarations context = List.filterMap (\decl -> case getNamedFunctionType "expect" decl of - Just ( name, Ok args ) -> + Just ( name, _, Ok args ) -> Just ( name, args ) _ -> @@ -58,13 +58,14 @@ collectExpectTypes declarations context = ) -getNamedFunctionType : String -> Node Declaration -> Maybe ( String, Result FunctionParseError (List (Node TypeAnnotation)) ) +getNamedFunctionType : String -> Node Declaration -> Maybe ( String, Node String, Result FunctionParseError (List (Node TypeAnnotation)) ) getNamedFunctionType prefix declaration = case getFunctionType declaration of Just ( functionName, annotation ) -> if String.startsWith prefix (Node.value functionName) then Just ( String.dropLeft (String.length prefix) (Node.value functionName) + , functionName , case Maybe.map (List.reverse << Nonempty.toList) annotation of Just (returnType :: programTest :: args) -> -- TODO: validate that return value is Expectation @@ -75,7 +76,7 @@ getNamedFunctionType prefix declaration = Err NotEnoughArgs Nothing -> - Err (NoTypeAnnotation functionName) + Err NoTypeAnnotation ) else @@ -88,7 +89,7 @@ getNamedFunctionType prefix declaration = type FunctionParseError = NotAFunction | NotEnoughArgs - | NoTypeAnnotation (Node String) + | NoTypeAnnotation getFunctionType : Node Declaration -> Maybe ( Node String, Maybe (Nonempty (Node TypeAnnotation)) ) @@ -123,7 +124,7 @@ flattenFunctionType typeAnnotation = validateEnsureTypes : Node Declaration -> Context -> ( List (Rule.Error {}), Context ) validateEnsureTypes node context = ( case getNamedFunctionType "ensure" node of - Just ( name, Ok ensureArgs ) -> + Just ( name, functionName, Ok ensureArgs ) -> case Dict.get name context.expectFunctionArguments of Just expectArgs -> let @@ -146,19 +147,30 @@ validateEnsureTypes node context = { message = "ensure" ++ name ++ " should take the same arguments as expect" ++ name , details = [ "Assuming the type annotation for expect" ++ name ++ " is correct, the type annotation for ensure" ++ name ++ " should be:" - , String.join " -> " (List.map typeAnnotationToString expectArgs) - ++ " -> ProgramTest msg model effect -> ProgramTest msg model effect" + , String.join " -> " + (List.map typeAnnotationToString expectArgs + ++ [ "ProgramTest msg model effect -> ProgramTest msg model effect" ] + ) ] } (Range.combine <| List.map Node.range mismatchedArgs) ] - _ -> - -- TODO: report when the corresponding expect function doesn't exist - -- TODO: report that ensure* must be a function - [] + Nothing -> + [ Rule.error + { message = "ensure" ++ name ++ " must have a corresponding expect" ++ name ++ " function" + , details = + [ "The type for expect" ++ name ++ " should be:" + , String.join " -> " + (List.map typeAnnotationToString ensureArgs + ++ [ "ProgramTest msg model effect -> Expectation" ] + ) + ] + } + (Node.range functionName) + ] - Just ( name, Err (NoTypeAnnotation functionName) ) -> + Just ( name, functionName, Err NoTypeAnnotation ) -> [ Rule.error { message = Node.value functionName ++ " must have a type annotation" , details = @@ -170,6 +182,7 @@ validateEnsureTypes node context = ] _ -> + -- TODO: report that ensure* must be a function [] , context ) diff --git a/review/tests/ExpectEnsurePairsMatchTest.elm b/review/tests/ExpectEnsurePairsMatchTest.elm index f55894c..27c8fcb 100644 --- a/review/tests/ExpectEnsurePairsMatchTest.elm +++ b/review/tests/ExpectEnsurePairsMatchTest.elm @@ -99,4 +99,27 @@ all = } |> Review.Test.atExactly { start = { row = 9, column = 1 }, end = { row = 9, column = 16 } } ] + , test "ensure function must have a corresponding expect function" <| + \() -> + String.join "\n" + [ "module ProgramTest exposing (ensureSomething)" + , "import Expect exposing (Expectation)" + , "" + , "type alias ProgramTest msg model effect = ()" + , "" + , "ensureSomething : String -> ProgramTest msg model effect -> ProgramTest msg model effect" + , "ensureSomething = Debug.todo \"expectSomething\"" + ] + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "ensureSomething must have a corresponding expectSomething function" + , details = + [ "The type for expectSomething should be:" + , "String -> ProgramTest msg model effect -> Expectation" + ] + , under = "ensureSomething" + } + |> Review.Test.atExactly { start = { row = 7, column = 1 }, end = { row = 7, column = 16 } } + ] ]