Backport work from review-common

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 _ ->
+            []

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 = "(..)"
+                            }
+                        ]
+        ]

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

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
+        ]

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 _ ->
+                    []
+
+        _ ->
+            []

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 <CustomTypeName>` 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
+        ]