The Credo experiment (#1740)

This PR adds credo and addresses the most important issues (and some low hanging fruit) raised when using `--strict`. This also adds it to our CI, but it doesn't fail it for now. I've also modified some of the settings: - disabled `{Credo.Check.Readability.ParenthesesOnZeroArityDefs, []}` as I don't know if I agree with this, at the very least we should discuss and decide if we want this enforced. (this check, if enabled, would require us to remove `()` from functions which don't accept any arguments) - disabled `{Credo.Check.Refactor.CondStatements, []}` as we have two 2-condition `cond` statements in `NervesHub.Extensions` which are useful to keep around. If we want to enforce this then we might need to change these until it makes sense switching back to using `cond` for them. - customized `{Credo.Check.Refactor.Nesting, [max_nesting: 3]}`, the default is 2 but we have enough 3 level nesting that I didn't feel like going down that path right now. - customized `Credo.Check.Warning.MissedMetadataKeyInLoggerConfig` to tell it that we do use `:all` in `runtime.exs` - customized `Credo.Check.Readability.ParenthesesOnZeroArityDefs` to require parentheses (the opposite of its default) - enabled `Credo.Check.Warning.LeakyEnvironment` to warn us if we are passing the full environment from the parent process when running system commands. - enabled `Credo.Check.Readability.ImplTrue` as @jjcarstens hates it when `@impl true` is used - and finally, enabled `Credo.Check.Readability.MultiAlias` as the use of `alias NervesHub.{Deployments, Devices, Products, ...}` is a code smell (@jjcarstens can swear to this) We have only `readability` and `software design suggestions` issues to address, or ignore. These primarily consist of: - `Modules should have a @moduledoc tag` - `Nested modules could be aliased at the top of the invoking module.` - and `Found a TODO tag in a comment` I think we should address all of these, but we can instead leave these for another PR. --------- Co-authored-by: Jon Carstens <[email protected]>
nerves-hub · Jan 11, 2025 · 2950802 · 2950802
1 parent 3d506ec
commit 2950802
Show file tree

Hide file tree

Showing 110 changed files with 768 additions and 385 deletions.
diff --git a/.credo.exs b/.credo.exs
@@ -0,0 +1,222 @@
+# This file contains the configuration for Credo and you are probably reading
+# this after creating it with `mix credo.gen.config`.
+#
+# If you find anything wrong or unclear in this file, please report an
+# issue on GitHub: https://github.com/rrrene/credo/issues
+#
+%{
+  #
+  # You can have as many configs as you like in the `configs:` field.
+  configs: [
+    %{
+      #
+      # Run any config using `mix credo -C <name>`. If no config name is given
+      # "default" is used.
+      #
+      name: "default",
+      #
+      # These are the files included in the analysis:
+      files: %{
+        #
+        # You can give explicit globs or simply directories.
+        # In the latter case `**/*.{ex,exs}` will be used.
+        #
+        included: [
+          "lib/",
+          "src/",
+          "test/",
+          "web/",
+          "apps/*/lib/",
+          "apps/*/src/",
+          "apps/*/test/",
+          "apps/*/web/"
+        ],
+        excluded: [~r"/_build/", ~r"/deps/", ~r"/node_modules/"]
+      },
+      #
+      # Load and configure plugins here:
+      #
+      plugins: [],
+      #
+      # If you create your own checks, you must specify the source files for
+      # them here, so they can be loaded by Credo before running the analysis.
+      #
+      requires: [],
+      #
+      # Start enforcing our evolving style guide.
+      #
+      strict: true,
+      #
+      # To modify the timeout for parsing files, change this value:
+      #
+      parse_timeout: 5000,
+      #
+      # If you want to use uncolored output by default, you can change `color`
+      # to `false` below:
+      #
+      color: true,
+      #
+      # You can customize the parameters of any check by adding a second element
+      # to the tuple.
+      #
+      # To disable a check put `false` as second element:
+      #
+      #     {Credo.Check.Design.DuplicatedCode, false}
+      #
+      checks: %{
+        enabled: [
+          #
+          ## Consistency Checks
+          #
+          {Credo.Check.Consistency.ExceptionNames, []},
+          {Credo.Check.Consistency.LineEndings, []},
+          {Credo.Check.Consistency.ParameterPatternMatching, []},
+          {Credo.Check.Consistency.SpaceAroundOperators, []},
+          {Credo.Check.Consistency.SpaceInParentheses, []},
+          {Credo.Check.Consistency.TabsOrSpaces, []},
+
+          #
+          ## Design Checks
+          #
+          # You can customize the priority of any check
+          # Priority values are: `low, normal, high, higher`
+          #
+          {Credo.Check.Design.AliasUsage,
+           [priority: :low, if_nested_deeper_than: 2, if_called_more_often_than: 0]},
+          {Credo.Check.Design.TagFIXME, []},
+          # You can also customize the exit_status of each check.
+          # If you don't want TODO comments to cause `mix credo` to fail, just
+          # set this value to 0 (zero).
+          #
+          {Credo.Check.Design.TagTODO, [exit_status: 0]},
+
+          #
+          ## Readability Checks
+          #
+          {Credo.Check.Readability.AliasOrder, []},
+          {Credo.Check.Readability.FunctionNames, []},
+          {Credo.Check.Readability.ImplTrue, []},
+          {Credo.Check.Readability.LargeNumbers, []},
+          {Credo.Check.Readability.MaxLineLength, [priority: :low, max_length: 120]},
+          {Credo.Check.Readability.ModuleAttributeNames, []},
+          {Credo.Check.Readability.ModuleDoc, []},
+          {Credo.Check.Readability.ModuleNames, []},
+          {Credo.Check.Readability.MultiAlias, []},
+          {Credo.Check.Readability.ParenthesesInCondition, []},
+          {Credo.Check.Readability.ParenthesesOnZeroArityDefs, [parens: true]},
+          {Credo.Check.Readability.PipeIntoAnonymousFunctions, []},
+          {Credo.Check.Readability.PredicateFunctionNames, []},
+          {Credo.Check.Readability.PreferImplicitTry, []},
+          {Credo.Check.Readability.RedundantBlankLines, []},
+          {Credo.Check.Readability.Semicolons, []},
+          {Credo.Check.Readability.SpaceAfterCommas, []},
+          {Credo.Check.Readability.StringSigils, []},
+          {Credo.Check.Readability.TrailingBlankLine, []},
+          {Credo.Check.Readability.TrailingWhiteSpace, []},
+          {Credo.Check.Readability.UnnecessaryAliasExpansion, []},
+          {Credo.Check.Readability.VariableNames, []},
+          {Credo.Check.Readability.WithSingleClause, []},
+
+          #
+          ## Refactoring Opportunities
+          #
+          {Credo.Check.Refactor.Apply, []},
+          # Disabling this as we use a two 2-statement conds in `NervesHub.Extensions`
+          # {Credo.Check.Refactor.CondStatements, []},
+          {Credo.Check.Refactor.CyclomaticComplexity, []},
+          {Credo.Check.Refactor.FilterCount, []},
+          {Credo.Check.Refactor.FilterFilter, []},
+          {Credo.Check.Refactor.FunctionArity, []},
+          {Credo.Check.Refactor.LongQuoteBlocks, []},
+          {Credo.Check.Refactor.MapJoin, []},
+          {Credo.Check.Refactor.MatchInCondition, []},
+          {Credo.Check.Refactor.NegatedConditionsInUnless, []},
+          {Credo.Check.Refactor.NegatedConditionsWithElse, []},
+          # Customized to 3, as we use a lot of 3 level nesting
+          {Credo.Check.Refactor.Nesting, [max_nesting: 3]},
+          {Credo.Check.Refactor.RedundantWithClauseResult, []},
+          {Credo.Check.Refactor.RejectReject, []},
+          {Credo.Check.Refactor.UnlessWithElse, []},
+          {Credo.Check.Refactor.WithClauses, []},
+
+          #
+          ## Warnings
+          #
+          {Credo.Check.Warning.ApplicationConfigInModuleAttribute, []},
+          {Credo.Check.Warning.BoolOperationOnSameValues, []},
+          {Credo.Check.Warning.Dbg, []},
+          {Credo.Check.Warning.ExpensiveEmptyEnumCheck, []},
+          {Credo.Check.Warning.IExPry, []},
+          {Credo.Check.Warning.IoInspect, []},
+          {Credo.Check.Warning.LeakyEnvironment, []},
+          # we use `:all` in `runtime.exs`, credo wasn't picking this up
+          {Credo.Check.Warning.MissedMetadataKeyInLoggerConfig,
+           [
+             metadata_keys: :all
+           ]},
+          {Credo.Check.Warning.OperationOnSameValues, []},
+          {Credo.Check.Warning.OperationWithConstantResult, []},
+          {Credo.Check.Warning.RaiseInsideRescue, []},
+          {Credo.Check.Warning.SpecWithStruct, []},
+          {Credo.Check.Warning.UnsafeExec, []},
+          {Credo.Check.Warning.UnusedEnumOperation, []},
+          {Credo.Check.Warning.UnusedFileOperation, []},
+          {Credo.Check.Warning.UnusedKeywordOperation, []},
+          {Credo.Check.Warning.UnusedListOperation, []},
+          {Credo.Check.Warning.UnusedPathOperation, []},
+          {Credo.Check.Warning.UnusedRegexOperation, []},
+          {Credo.Check.Warning.UnusedStringOperation, []},
+          {Credo.Check.Warning.UnusedTupleOperation, []},
+          {Credo.Check.Warning.WrongTestFileExtension, []}
+        ],
+        disabled: [
+          #
+          # Checks scheduled for next check update (opt-in for now)
+          {Credo.Check.Refactor.UtcNowTruncate, []},
+
+          #
+          # Controversial and experimental checks (opt-in, just move the check to `:enabled`
+          #   and be sure to use `mix credo --strict` to see low priority checks)
+          #
+          {Credo.Check.Consistency.MultiAliasImportRequireUse, []},
+          {Credo.Check.Consistency.UnusedVariableNames, []},
+          {Credo.Check.Design.DuplicatedCode, []},
+          {Credo.Check.Design.SkipTestWithoutComment, []},
+          {Credo.Check.Readability.AliasAs, []},
+          {Credo.Check.Readability.BlockPipe, []},
+          {Credo.Check.Readability.NestedFunctionCalls, []},
+          {Credo.Check.Readability.OneArityFunctionInPipe, []},
+          {Credo.Check.Readability.OnePipePerLine, []},
+          {Credo.Check.Readability.SeparateAliasRequire, []},
+          {Credo.Check.Readability.SingleFunctionToBlockPipe, []},
+          {Credo.Check.Readability.SinglePipe, []},
+          {Credo.Check.Readability.Specs, []},
+          {Credo.Check.Readability.StrictModuleLayout, []},
+          {Credo.Check.Readability.WithCustomTaggedTuple, []},
+          {Credo.Check.Refactor.ABCSize, []},
+          {Credo.Check.Refactor.AppendSingleItem, []},
+          {Credo.Check.Refactor.DoubleBooleanNegation, []},
+          {Credo.Check.Refactor.FilterReject, []},
+          {Credo.Check.Refactor.IoPuts, []},
+          {Credo.Check.Refactor.MapMap, []},
+          {Credo.Check.Refactor.ModuleDependencies, []},
+          {Credo.Check.Refactor.NegatedIsNil, []},
+          {Credo.Check.Refactor.PassAsyncInTestCases, []},
+          {Credo.Check.Refactor.PipeChainStart, []},
+          {Credo.Check.Refactor.RejectFilter, []},
+          {Credo.Check.Refactor.VariableRebinding, []},
+          {Credo.Check.Warning.LazyLogging, []},
+          {Credo.Check.Warning.MapGetUnsafePass, []},
+          {Credo.Check.Warning.MixEnv, []},
+          {Credo.Check.Warning.UnsafeToAtom, []}
+
+          # {Credo.Check.Refactor.MapInto, []},
+
+          #
+          # Custom checks can be created using `mix credo.gen.check`.
+          #
+        ]
+      }
+    }
+  ]
+}
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -92,6 +92,9 @@ jobs:
       - name: Check for unused dependencies
         run: mix deps.unlock --unused
 
+      - name: Run Credo (won't fail the build)
+        run: mix credo --mute-exit-status
+
       - name: DB Setup
         run: mix ecto.migrate.reset
 

diff --git a/lib/mix/tasks/assets.build.ex b/lib/mix/tasks/assets.build.ex
@@ -1,4 +1,6 @@
 defmodule Mix.Tasks.Assets.Build do
+  @moduledoc false
+
   use Mix.Task
 
   @shortdoc "Build web assets"
@@ -10,7 +12,8 @@ defmodule Mix.Tasks.Assets.Build do
       ["run", "deploy"],
       cd: @assets,
       stderr_to_stdout: true,
-      into: IO.stream(:stdio, :line)
+      into: IO.stream(:stdio, :line),
+      env: []
     )
   end
 end
diff --git a/lib/mix/tasks/assets.install.ex b/lib/mix/tasks/assets.install.ex
@@ -1,4 +1,6 @@
 defmodule Mix.Tasks.Assets.Install do
+  @moduledoc false
+
   use Mix.Task
 
   @shortdoc "Install web assets"
@@ -10,7 +12,8 @@ defmodule Mix.Tasks.Assets.Install do
       ["install"],
       cd: @assets,
       stderr_to_stdout: true,
-      into: IO.stream(:stdio, :line)
+      into: IO.stream(:stdio, :line),
+      env: []
     )
   end
 end
diff --git a/lib/mix/tasks/fake.metrics.ex b/lib/mix/tasks/fake.metrics.ex
@@ -1,10 +1,12 @@
 if Mix.env() == :dev do
   defmodule Mix.Tasks.Fake.Metrics do
+    @moduledoc false
+
     use Mix.Task
 
-    alias NervesHub.Repo
     alias NervesHub.Devices
     alias NervesHub.Devices.DeviceMetric
+    alias NervesHub.Repo
 
     @shortdoc "Create randomized metrics for device"
     @requirements ["app.start"]

diff --git a/lib/mix/tasks/gen.devices.ex b/lib/mix/tasks/gen.devices.ex
@@ -1,4 +1,6 @@
 defmodule Mix.Tasks.Gen.Devices do
+  @moduledoc false
+
   use Mix.Task
 
   @shortdoc "Generate a mass of devices"

diff --git a/lib/nerves_hub/accounts.ex b/lib/nerves_hub/accounts.ex
@@ -1,19 +1,18 @@
 defmodule NervesHub.Accounts do
   import Ecto.Query
 
-  alias Ecto.{Changeset, Multi}
+  alias Ecto.Changeset
+  alias Ecto.Multi
   alias Ecto.UUID
 
-  alias NervesHub.Accounts.{
-    Org,
-    User,
-    UserToken,
-    Invite,
-    OrgKey,
-    OrgUser,
-    OrgMetric,
-    RemoveAccount
-  }
+  alias NervesHub.Accounts.Invite
+  alias NervesHub.Accounts.Org
+  alias NervesHub.Accounts.OrgKey
+  alias NervesHub.Accounts.OrgMetric
+  alias NervesHub.Accounts.OrgUser
+  alias NervesHub.Accounts.RemoveAccount
+  alias NervesHub.Accounts.User
+  alias NervesHub.Accounts.UserToken
 
   alias NervesHub.Repo
 

diff --git a/lib/nerves_hub/accounts/org.ex b/lib/nerves_hub/accounts/org.ex
@@ -4,8 +4,10 @@ defmodule NervesHub.Accounts.Org do
   import Ecto.Changeset
   import Ecto.Query
 
-  alias NervesHub.Accounts.{OrgKey, OrgUser}
-  alias NervesHub.Devices.{Device, CACertificate}
+  alias NervesHub.Accounts.OrgKey
+  alias NervesHub.Accounts.OrgUser
+  alias NervesHub.Devices.CACertificate
+  alias NervesHub.Devices.Device
   alias NervesHub.Products.Product
   alias NervesHub.Repo
   alias __MODULE__

diff --git a/lib/nerves_hub/accounts/org_key.ex b/lib/nerves_hub/accounts/org_key.ex
@@ -3,7 +3,8 @@ defmodule NervesHub.Accounts.OrgKey do
 
   import Ecto.Changeset
 
-  alias NervesHub.Accounts.{Org, User}
+  alias NervesHub.Accounts.Org
+  alias NervesHub.Accounts.User
   alias NervesHub.Firmwares.Firmware
   alias __MODULE__
 

diff --git a/lib/nerves_hub/accounts/org_user.ex b/lib/nerves_hub/accounts/org_user.ex
@@ -3,7 +3,8 @@ defmodule NervesHub.Accounts.OrgUser do
 
   import Ecto.Query
 
-  alias NervesHub.Accounts.{User, Org}
+  alias NervesHub.Accounts.Org
+  alias NervesHub.Accounts.User
 
   @type t :: %__MODULE__{}