From a6870989a1d8ef4c1052b7bd798558de4bd825cf Mon Sep 17 00:00:00 2001 From: Dillon Kearns Date: Thu, 24 Mar 2022 15:18:28 -0700 Subject: [PATCH] Report missing core modules in NoContractViolations review check. --- src/Pages/Review/NoContractViolations.elm | 91 +++++--- .../Pages/Review/NoContractViolationsTest.elm | 198 +++++++++++++----- 2 files changed, 207 insertions(+), 82 deletions(-) diff --git a/src/Pages/Review/NoContractViolations.elm b/src/Pages/Review/NoContractViolations.elm index 100b7508..5ffe9b6b 100644 --- a/src/Pages/Review/NoContractViolations.elm +++ b/src/Pages/Review/NoContractViolations.elm @@ -52,13 +52,54 @@ elm-review --template dillonkearns/elm-review-elm-pages/example --rules Pages.Re -} rule : Rule rule = - Rule.newModuleRuleSchema "Pages.Review.NoContractViolations" - { moduleName = [] - , isRouteModule = False + Rule.newProjectRuleSchema "Pages.Review.NoContractViolations" + { visitedCoreModules = Set.empty } - |> Rule.withModuleDefinitionVisitor moduleDefinitionVisitor - |> Rule.withDeclarationVisitor declarationVisitor - |> Rule.fromModuleRuleSchema + |> Rule.withModuleVisitor + (Rule.withModuleDefinitionVisitor moduleDefinitionVisitor + >> Rule.withDeclarationVisitor declarationVisitor + ) + |> Rule.withModuleContext + { foldProjectContexts = \a b -> { visitedCoreModules = Set.union a.visitedCoreModules b.visitedCoreModules } + , fromModuleToProject = \moduleKey moduleName moduleContext -> { visitedCoreModules = Set.singleton (Node.value moduleName) } + , fromProjectToModule = + \moduleKey moduleName projectContext -> + { moduleName = Node.value moduleName + , isRouteModule = (Node.value moduleName |> List.take 1) == [ "Route" ] && ((Node.value moduleName |> List.length) > 1) + } + } + |> Rule.withFinalProjectEvaluation + (\context -> + let + missingCoreModules = + context.visitedCoreModules + |> Set.diff coreModules + in + if missingCoreModules |> Set.isEmpty then + [] + + else + [ Rule.globalError + { message = "Missing core modules" + , details = + missingCoreModules + |> Set.toList + |> List.map (String.join ".") + } + ] + ) + |> Rule.fromProjectRuleSchema + + +coreModules : Set (List String) +coreModules = + Set.fromList + [ [ "Api" ] + , [ "Effect" ] + , [ "Shared" ] + , [ "Site" ] + , [ "View" ] + ] type alias Context = @@ -67,32 +108,22 @@ type alias Context = } +type alias ProjectContext = + { visitedCoreModules : Set (List String) + } + + moduleDefinitionVisitor : Node Module -> Context -> ( List (Error {}), Context ) -moduleDefinitionVisitor node _ = - let - isRouteModule : Bool - isRouteModule = - (Node.value node |> Module.moduleName |> List.take 1) - == [ "Route" ] - && ((Node.value node |> Module.moduleName |> List.length) > 1) - in +moduleDefinitionVisitor node context = case Node.value node |> Module.exposingList of Exposing.All _ -> - ( [] - , { moduleName = Node.value node |> Module.moduleName - , isRouteModule = isRouteModule - } - ) + ( [], context ) Exposing.Explicit exposedValues -> - if isRouteModule then + if context.isRouteModule then case Set.diff (Set.fromList [ "Data", "Msg", "Model", "route" ]) (exposedNames exposedValues) |> Set.toList of [] -> - ( [] - , { moduleName = Node.value node |> Module.moduleName - , isRouteModule = isRouteModule - } - ) + ( [], context ) nonEmpty -> ( [ Rule.error @@ -111,17 +142,11 @@ But it is not exposing: """ } (Node.range (exposingListNode (Node.value node))) ] - , { moduleName = Node.value node |> Module.moduleName - , isRouteModule = isRouteModule - } + , context ) else - ( [] - , { moduleName = Node.value node |> Module.moduleName - , isRouteModule = isRouteModule - } - ) + ( [], context ) routeParamsMatchesNameOrError : Node TypeAnnotation -> List String -> List (Error {}) diff --git a/tests/Pages/Review/NoContractViolationsTest.elm b/tests/Pages/Review/NoContractViolationsTest.elm index 18c986f7..aff1eaa6 100644 --- a/tests/Pages/Review/NoContractViolationsTest.elm +++ b/tests/Pages/Review/NoContractViolationsTest.elm @@ -14,12 +14,13 @@ all = a = 1 """ - |> Review.Test.run rule - |> Review.Test.expectErrors - [ Review.Test.error - { message = "Unexposed Declaration in Route Module" - , details = - [ """Route Modules need to expose the following values: + |> testRouteModule + |> Review.Test.expectErrorsForModules + [ ( "Route.Blog.Slug_" + , [ Review.Test.error + { message = "Unexposed Declaration in Route Module" + , details = + [ """Route Modules need to expose the following values: - route - Data @@ -27,9 +28,11 @@ a = 1 - Msg But it is not exposing: Model""" - ] - , under = "exposing (Data, Msg, route)" - } + ] + , under = "exposing (Data, Msg, route)" + } + ] + ) ] , test "reports RouteParams mismatch" <| \() -> @@ -39,18 +42,21 @@ type alias RouteParams = { blogPostName : String } route = {} """ - |> Review.Test.run rule - |> Review.Test.expectErrors - [ Review.Test.error - { message = "RouteParams don't match Route Module name" - , details = - [ """Expected + |> testRouteModule + |> Review.Test.expectErrorsForModules + [ ( "Route.Blog.Slug_" + , [ Review.Test.error + { message = "RouteParams don't match Route Module name" + , details = + [ """Expected type alias RouteParams = { slug : String } """ - ] - , under = "{ blogPostName : String }" - } + ] + , under = "{ blogPostName : String }" + } + ] + ) ] , test "reports incorrect types for optional RouteParams" <| \() -> @@ -60,18 +66,21 @@ type alias RouteParams = { section : String, subSection : String } route = {} """ - |> Review.Test.run rule - |> Review.Test.expectErrors - [ Review.Test.error - { message = "RouteParams don't match Route Module name" - , details = - [ """Expected + |> testRouteModule + |> Review.Test.expectErrorsForModules + [ ( "Route.Docs.Section_.SubSection__" + , [ Review.Test.error + { message = "RouteParams don't match Route Module name" + , details = + [ """Expected type alias RouteParams = { section : String, subSection : Maybe String } """ - ] - , under = "{ section : String, subSection : String }" - } + ] + , under = "{ section : String, subSection : String }" + } + ] + ) ] , test "reports incorrect types for required splat RouteParams" <| \() -> @@ -81,18 +90,21 @@ type alias RouteParams = { section : String, splat : List String } route = {} """ - |> Review.Test.run rule - |> Review.Test.expectErrors - [ Review.Test.error - { message = "RouteParams don't match Route Module name" - , details = - [ """Expected + |> testRouteModule + |> Review.Test.expectErrorsForModules + [ ( "Route.Docs.Section_.SPLAT_" + , [ Review.Test.error + { message = "RouteParams don't match Route Module name" + , details = + [ """Expected type alias RouteParams = { section : String, splat : ( String, List String ) } """ - ] - , under = "{ section : String, splat : List String }" - } + ] + , under = "{ section : String, splat : List String }" + } + ] + ) ] , test "no error for valid SPLAT_ RouteParams" <| \() -> @@ -102,7 +114,7 @@ type alias RouteParams = { section : String, splat : ( String, List String ) } route = {} """ - |> Review.Test.run rule + |> testRouteModule |> Review.Test.expectNoErrors , test "no error for valid SPLAT__ RouteParams" <| \() -> @@ -112,7 +124,7 @@ type alias RouteParams = { section : String, splat : List String } route = {} """ - |> Review.Test.run rule + |> testRouteModule |> Review.Test.expectNoErrors , test "no error for matching RouteParams name" <| \() -> @@ -122,7 +134,7 @@ type alias RouteParams = { slug : String } route = {} """ - |> Review.Test.run rule + |> testRouteModule |> Review.Test.expectNoErrors , test "error when RouteParams type is not a record" <| \() -> @@ -132,15 +144,18 @@ type alias RouteParams = () route = {} """ - |> Review.Test.run rule - |> Review.Test.expectErrors - [ Review.Test.error - { message = "RouteParams must be a record type alias." - , details = - [ """Expected a record type alias.""" - ] - , under = "()" - } + |> testRouteModule + |> Review.Test.expectErrorsForModules + [ ( "Route.Blog.Slug_" + , [ Review.Test.error + { message = "RouteParams must be a record type alias." + , details = + [ """Expected a record type alias.""" + ] + , under = "()" + } + ] + ) ] , test "no error for modules that don't start with Route prefix" <| \() -> @@ -150,6 +165,91 @@ type alias RouteParams = () route = {} """ - |> Review.Test.run rule + |> testRouteModule + |> Review.Test.expectNoErrors + , test "error for missing application module definitions" <| + \() -> + [ """module Route.Index exposing (Data, route, Model, Msg) + +type alias RouteParams = {} + +route = {} +""" + , """module Site exposing (config) + +config : SiteConfig +config = + { canonicalUrl = canonicalUrl + , head = head + } +""" + ] + |> Review.Test.runOnModules rule + |> Review.Test.expectGlobalErrors + [ { message = "Missing core modules" + , details = + [ "Api" + , "Effect" + , "Shared" + , "View" + ] + } + ] + , test "no error when all core modules are defined" <| + \() -> + [ """module Route.Index exposing (Data, route, Model, Msg) + +type alias RouteParams = {} + +route = {} +""" + , """module Site exposing (config) + +config : SiteConfig +config = + { canonicalUrl = canonicalUrl + , head = head + } +""" + , """module Api exposing (routes) +routes = Debug.todo "" +""" + , """module Effect exposing (routes) +routes = Debug.todo "" +""" + , """module Shared exposing (routes) +routes = Debug.todo "" +""" + , """module View exposing (routes) +routes = Debug.todo "" +""" + ] + |> Review.Test.runOnModules rule |> Review.Test.expectNoErrors ] + + +testRouteModule : String -> Review.Test.ReviewResult +testRouteModule routeModule = + Review.Test.runOnModules rule + (routeModule :: validCoreModules) + + +validCoreModules : List String +validCoreModules = + [ """module Api exposing (routes) +routes = Debug.todo "" +""" + , """module Effect exposing (routes) +routes = Debug.todo "" +""" + , """module Shared exposing (routes) +routes = Debug.todo "" +""" + , """module Site exposing (routes) +routes = Debug.todo "" +""" + , """module View exposing (routes) +routes = Debug.todo "" +""" + ]