From 1fbf67bddb27274dd8f861c1e4e1fffffd4a3aa5 Mon Sep 17 00:00:00 2001 From: Jeroen Engels Date: Wed, 8 Apr 2020 00:14:03 +0200 Subject: [PATCH] Backport work from review-common --- tests/NoExposingEverything.elm | 67 +++++++++++ tests/NoExposingEverythingTest.elm | 29 +++++ tests/NoImportingEverything.elm | 115 +++++++++---------- tests/NoImportingEverythingTest.elm | 158 ++++++++------------------ tests/NoMissingTypeAnnotation.elm | 79 +++++++++++++ tests/NoMissingTypeAnnotationTest.elm | 63 ++++++++++ 6 files changed, 339 insertions(+), 172 deletions(-) create mode 100644 tests/NoExposingEverything.elm create mode 100644 tests/NoExposingEverythingTest.elm create mode 100644 tests/NoMissingTypeAnnotation.elm create mode 100644 tests/NoMissingTypeAnnotationTest.elm diff --git a/tests/NoExposingEverything.elm b/tests/NoExposingEverything.elm new file mode 100644 index 00000000..0f5eb2ce --- /dev/null +++ b/tests/NoExposingEverything.elm @@ -0,0 +1,67 @@ +module NoExposingEverything exposing (rule) + +{-| + +@docs rule + +-} + +import Elm.Syntax.Exposing as Exposing +import Elm.Syntax.Module as Module exposing (Module) +import Elm.Syntax.Node as Node exposing (Node) +import Review.Rule as Rule exposing (Error, Rule) + + +{-| Forbids exporting everything from a module. + +Modules should have hidden implementation details with an explicit API so that the module is used in a proper and controlled way. +The users of this module should not have to know about what is inside a module it is using, and they shouldn't need to access its internal details. +Therefore, the API should be explicitly defined and ideally as small as possible. + + config = + [ NoExposingEverything.rule + ] + +If you would like to expose everything in your tests, you can configure the rule +in the following manner: + + config = + [ NoExposingEverything.rule + |> Rule.ignoreErrorsForDirectories [ "tests/" ] + ] + + +## Fail + + module A exposing (..) + + +## Success + + module A exposing (B(..), C, d) + +-} +rule : Rule +rule = + Rule.newModuleRuleSchema "NoExposingEverything" () + |> Rule.withSimpleModuleDefinitionVisitor moduleDefinitionVisitor + |> Rule.fromModuleRuleSchema + + +moduleDefinitionVisitor : Node Module -> List (Error {}) +moduleDefinitionVisitor moduleNode = + case Module.exposingList <| Node.value moduleNode of + Exposing.All range -> + [ Rule.error + { message = "Module exposes everything implicitly \"(..)\"" + , details = + [ "Modules should have hidden implementation details with an explicit API so that the module is used in a proper and controlled way. The users of this module should not have to know about what is inside a module it is using, and they shouldn't need to access it's internal details. Therefore, the API should be explicitly defined and ideally as small as possible." + ] + } + { start = { row = range.start.row, column = range.start.column - 1 } + , end = { row = range.end.row, column = range.end.column + 1 } + } + ] + + Exposing.Explicit _ -> + [] diff --git a/tests/NoExposingEverythingTest.elm b/tests/NoExposingEverythingTest.elm new file mode 100644 index 00000000..fefd07f8 --- /dev/null +++ b/tests/NoExposingEverythingTest.elm @@ -0,0 +1,29 @@ +module NoExposingEverythingTest exposing (all) + +import NoExposingEverything exposing (rule) +import Review.Test +import Test exposing (..) + + +all : Test +all = + describe "NoExposingEverything" + [ test "should not when a module exposes a limited set of things" <| + \_ -> + "module A exposing (B(..), C, d)" + |> Review.Test.run rule + |> Review.Test.expectNoErrors + , test "should report when a module exposes everything" <| + \_ -> + "module A exposing (..)" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Module exposes everything implicitly \"(..)\"" + , details = + [ "Modules should have hidden implementation details with an explicit API so that the module is used in a proper and controlled way. The users of this module should not have to know about what is inside a module it is using, and they shouldn't need to access it's internal details. Therefore, the API should be explicitly defined and ideally as small as possible." + ] + , under = "(..)" + } + ] + ] diff --git a/tests/NoImportingEverything.elm b/tests/NoImportingEverything.elm index 929c832f..43ef1b7e 100644 --- a/tests/NoImportingEverything.elm +++ b/tests/NoImportingEverything.elm @@ -1,103 +1,92 @@ -module NoImportingEverything exposing (rule, Configuration) +module NoImportingEverything exposing (rule) -{-| Forbid importing everything from a module. +{-| - -# Rule and configuration - -@docs rule, Configuration +@docs rule -} import Elm.Syntax.Exposing as Exposing import Elm.Syntax.Import exposing (Import) import Elm.Syntax.Node as Node exposing (Node) -import Elm.Syntax.Range exposing (Range) import Review.Rule as Rule exposing (Error, Rule) +import Set exposing (Set) -{-| Configuration for the rule. --} -type alias Configuration = - { exceptions : List String } +{-| Forbids importing everything from a module. - -{-| Forbid importing everything from a module. Doing so can be confusing, -especially to newcomers when the exposed functions and types are unknown to them. - -A preferred pattern is to import functions by name (`import Html exposing (div, span)`) -or using qualified imports (`import Html`, then `Html.div`). If the module name -is too long, don't forget that you can do qualified imports using an alias -(`import Html.Attributes as Attr`). - -You can make exceptions for some modules by adding them to the `exceptions` -field, like `{ exceptions = [ "Html", "Html.Attributes" ] }`. The name should be -the exact name of the import. Allowing importing everything from `Html` will not -allow the same thing for `Html.Events`, unless explicitly specified. +When you import everything from a module, it becomes harder to know where a function +or a type comes from. The official guide even +[recommends against importing everything](https://guide.elm-lang.org/webapps/modules.html#using-modules). config = - [ NoImportingEverything.rule { exceptions = [] } + [ NoImportingEverything.rule [] + ] + +Teams often have an agreement on the list of imports from which it is okay to expose everything, so +you can configure a list of exceptions. + + config = + [ NoImportingEverything.rule [ "Html", "Some.Module" ] ] ## Fail - import Html exposing (..) + import A exposing (..) + import A as B exposing (..) ## Success - -- NoImportingEverything.rule { exceptions = [] } - import Html exposing (div, p, textarea) + import A as B exposing (B(..), C, d) - -- NoImportingEverything.rule { exceptions = [ "Html" ] } + -- If configured with `[ "Html" ]` import Html exposing (..) - -# When not to use this rule - -If you prefer importing most of your modules using `exposing (..)`, then you -should not use this rule. - -} -rule : Configuration -> Rule -rule config = +rule : List String -> Rule +rule exceptions = Rule.newModuleRuleSchema "NoImportingEverything" () - |> Rule.withSimpleImportVisitor (importVisitor config) + |> Rule.withSimpleImportVisitor (importVisitor <| exceptionsToSet exceptions) |> Rule.fromModuleRuleSchema -error : Range -> String -> Error {} -error range name = - Rule.error - { message = "Do not expose everything from " ++ name - , details = - [ "Exposing `(..)` from a module means making all its exposed functions and types available in the file's namespace. This makes it hard to tell which module a function or type comes from." - , "A preferred pattern is to import functions by name (`import Html exposing (div, span)`) or to use qualified imports (`import Html`, then `Html.div`). If the module name is too long, you can give an alias to the imported module (`import Html.Attributes as Attr`)." - ] - } - range +exceptionsToSet : List String -> Set (List String) +exceptionsToSet exceptions = + exceptions + |> List.map (String.split ".") + |> Set.fromList -importVisitor : Configuration -> Node Import -> List (Error {}) -importVisitor config node = - let - { moduleName, exposingList } = - Node.value node - - name : String - name = - moduleName - |> Node.value - |> String.join "." - in - if List.member name config.exceptions then +importVisitor : Set (List String) -> Node Import -> List (Error {}) +importVisitor exceptions node = + if Set.member (moduleName node) exceptions then [] else - case exposingList |> Maybe.map Node.value of + case + Node.value node + |> .exposingList + |> Maybe.map Node.value + of Just (Exposing.All range) -> - [ error range name ] + [ Rule.error + { message = "Prefer listing what you wish to import and/or using qualified imports" + , details = [ "When you import everything from a module, it becomes harder to know where a function or a type comes from" ] + } + { start = { row = range.start.row, column = range.start.column - 1 } + , end = { row = range.end.row, column = range.end.column + 1 } + } + ] _ -> [] + + +moduleName : Node Import -> List String +moduleName node = + node + |> Node.value + |> .moduleName + |> Node.value diff --git a/tests/NoImportingEverythingTest.elm b/tests/NoImportingEverythingTest.elm index bfd94318..373c961e 100644 --- a/tests/NoImportingEverythingTest.elm +++ b/tests/NoImportingEverythingTest.elm @@ -1,120 +1,60 @@ module NoImportingEverythingTest exposing (all) -import NoImportingEverything exposing (Configuration, rule) -import Review.Test exposing (ReviewResult) -import Test exposing (Test, describe, test) - - -testRule : Configuration -> String -> ReviewResult -testRule options = - Review.Test.run (rule options) +import NoImportingEverything exposing (rule) +import Review.Test +import Test exposing (..) details : List String details = - [ "Exposing `(..)` from a module means making all its exposed functions and types available in the file's namespace. This makes it hard to tell which module a function or type comes from." - , "A preferred pattern is to import functions by name (`import Html exposing (div, span)`) or to use qualified imports (`import Html`, then `Html.div`). If the module name is too long, you can give an alias to the imported module (`import Html.Attributes as Attr`)." - ] - - -tests : List Test -tests = - [ test "should not report imports that do not expose anything" <| - \() -> - """module A exposing (..) -import Html -import Http""" - |> testRule { exceptions = [] } - |> Review.Test.expectNoErrors - , test "should not report imports that expose functions by name" <| - \() -> - """module A exposing (..) -import Html exposing (a) -import Http exposing (a, b)""" - |> testRule { exceptions = [] } - |> Review.Test.expectNoErrors - , test "should report imports that expose everything" <| - \() -> - """module A exposing (..) -import Html exposing (..)""" - |> testRule { exceptions = [] } - |> Review.Test.expectErrors - [ Review.Test.error - { message = "Do not expose everything from Html" - , details = details - , under = ".." - } - |> Review.Test.atExactly { start = { row = 2, column = 23 }, end = { row = 2, column = 25 } } - ] - , test "should report imports from sub-modules" <| - \() -> - """module A exposing (a) -import Html.App exposing (..)""" - |> testRule { exceptions = [] } - |> Review.Test.expectErrors - [ Review.Test.error - { message = "Do not expose everything from Html.App" - , details = details - , under = ".." - } - ] - , test "should report imports from sub-modules (multiple dots)" <| - \() -> - """module A exposing (a) -import Html.Foo.Bar exposing (..)""" - |> testRule { exceptions = [] } - |> Review.Test.expectErrors - [ Review.Test.error - { message = "Do not expose everything from Html.Foo.Bar" - , details = details - , under = ".." - } - ] - , test "should not report imports that expose everything that are in the exception list" <| - \() -> - """module A exposing (a) -import Html exposing (..)""" - |> testRule { exceptions = [ "Html" ] } - |> Review.Test.expectNoErrors - , test "should not report imports from sub-modules that are in the exception list" <| - \() -> - """module A exposing (a) -import Html.App exposing (..)""" - |> testRule { exceptions = [ "Html.App" ] } - |> Review.Test.expectNoErrors - , test "should not report imports from sub-modules (multiple dots)" <| - \() -> - """module A exposing (a) -import Html.Foo.Bar exposing (..)""" - |> testRule { exceptions = [ "Html.Foo.Bar" ] } - |> Review.Test.expectNoErrors - , test "should report imports whose parent is ignored" <| - \() -> - """module A exposing (a) -import Html.Foo.Bar exposing (..)""" - |> testRule { exceptions = [ "Html" ] } - |> Review.Test.expectErrors - [ Review.Test.error - { message = "Do not expose everything from Html.Foo.Bar" - , details = details - , under = ".." - } - ] - , test "should report imports whose sub-module is ignored" <| - \() -> - """module A exposing (a) -import Html exposing (..)""" - |> testRule { exceptions = [ "Html.App" ] } - |> Review.Test.expectErrors - [ Review.Test.error - { message = "Do not expose everything from Html" - , details = details - , under = ".." - } - ] + [ "Type annotations help you understand what happens in the code, and it will help the compiler give better error messages." ] all : Test all = - describe "NoImportingEverything" tests + describe "NoImportingEverything" + [ test "should not report imports without exposing clause" <| + \_ -> + """module A exposing (thing) +import Html +import Html as B +""" + |> Review.Test.run (rule []) + |> Review.Test.expectNoErrors + , test "should not report imports that expose some elements" <| + \_ -> + """module A exposing (thing) +import Html exposing (B, c) +""" + |> Review.Test.run (rule []) + |> Review.Test.expectNoErrors + , test "should not report imports that expose all constructors of a type" <| + \_ -> + """module A exposing (thing) +import Html exposing (B(..)) +""" + |> Review.Test.run (rule []) + |> Review.Test.expectNoErrors + , test "should report imports that expose everything" <| + \_ -> + """module A exposing (thing) +import Html exposing (..) +""" + |> Review.Test.run (rule []) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Prefer listing what you wish to import and/or using qualified imports" + , details = [ "When you import everything from a module, it becomes harder to know where a function or a type comes from" ] + , under = "(..)" + } + ] + , test "should not report imports that are in the exceptions list" <| + \_ -> + """module A exposing (thing) +import Html exposing (..) +import Thing.Foo as Foo exposing (..) +""" + |> Review.Test.run (rule [ "Html", "Thing.Foo" ]) + |> Review.Test.expectNoErrors + ] diff --git a/tests/NoMissingTypeAnnotation.elm b/tests/NoMissingTypeAnnotation.elm new file mode 100644 index 00000000..263b41e9 --- /dev/null +++ b/tests/NoMissingTypeAnnotation.elm @@ -0,0 +1,79 @@ +module NoMissingTypeAnnotation exposing (rule) + +{-| + +@docs rule + +-} + +import Elm.Syntax.Declaration as Declaration exposing (Declaration) +import Elm.Syntax.Node as Node exposing (Node) +import Review.Rule as Rule exposing (Error, Rule) + + +{-| Reports top-level declarations that do not have a type annotation. + +Type annotations help you understand what happens in the code, and it will help the compiler give better error messages. + + config = + [ NoMissingTypeAnnotation.rule + ] + +This rule does not report declarations without a type annotation inside a `let in`. + + +## Fail + + a = + 1 + + +## Success + + a : number + a = + 1 + + b : number + b = + let + c = + 2 + in + c + +-} +rule : Rule +rule = + Rule.newModuleRuleSchema "NoMissingTypeAnnotation" () + |> Rule.withSimpleDeclarationVisitor declarationVisitor + |> Rule.fromModuleRuleSchema + + +declarationVisitor : Node Declaration -> List (Error {}) +declarationVisitor declaration = + case Node.value declaration of + Declaration.FunctionDeclaration function -> + case function.signature of + Nothing -> + let + name : Node String + name = + function.declaration + |> Node.value + |> .name + in + [ Rule.error + { message = "Missing type annotation for `" ++ Node.value name ++ "`" + , details = + [ "Type annotations help you understand what happens in the code, and it will help the compiler give better error messages." + ] + } + (Node.range name) + ] + + Just _ -> + [] + + _ -> + [] diff --git a/tests/NoMissingTypeAnnotationTest.elm b/tests/NoMissingTypeAnnotationTest.elm new file mode 100644 index 00000000..d13e2e18 --- /dev/null +++ b/tests/NoMissingTypeAnnotationTest.elm @@ -0,0 +1,63 @@ +module NoMissingTypeAnnotationTest exposing (all) + +import NoMissingTypeAnnotation exposing (rule) +import Review.Test +import Test exposing (..) + + +details : List String +details = + [ "Type annotations help you understand what happens in the code, and it will help the compiler give better error messages." + ] + + +all : Test +all = + describe "NoMissingTypeAnnotation" + [ test "should not report anything when all top-level declarations have a type signature" <| + \_ -> + """module A exposing (..) +hasTypeAnnotation : Int +hasTypeAnnotation = 1 + +alsoHasTypeAnnotation : String -> List Things +alsoHasTypeAnnotation = doSomething +""" + |> Review.Test.run rule + |> Review.Test.expectNoErrors + , test "should report when a declaration named `all...` that is of type `List ` does not have all the type constructors in its value (1)" <| + \_ -> + """module A exposing (..) +hasNoTypeAnnotation = 1 +""" + |> Review.Test.run rule + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Missing type annotation for `hasNoTypeAnnotation`" + , details = details + , under = "hasNoTypeAnnotation" + } + ] + , test "should not report anything for custom type declarations" <| + \_ -> + """module A exposing (..) +type A = B | C +""" + |> Review.Test.run rule + |> Review.Test.expectNoErrors + , test "should not report anything for type alias declarations" <| + \_ -> + """module A exposing (..) +type alias A = { a : String } +""" + |> Review.Test.run rule + |> Review.Test.expectNoErrors + , test "should not report anything for port declarations" <| + \_ -> + """module A exposing (..) +port toJavaScript : Int -> Cmd msg +port fromJavaScript : (Int -> msg) -> Sub msg +""" + |> Review.Test.run rule + |> Review.Test.expectNoErrors + ]