Merge pull request beyond-all-reason#903 from dmarcoux/fix-credo-issues

Fix Credo issues

Author: L-e-x-o-n

.credo.exs

@@ -6,7 +6,10 @@
         {Credo.Check.Warning.MissedMetadataKeyInLoggerConfig,
          [
            metadata_keys: [:request_id, :user_id, :pid, :actor_type, :actor_id]
-         ]}
+         ]},
+        # TODO: Enable this check and fix the issues
+        {Credo.Check.Design.AliasUsage, false}
+        # {Credo.Check.Design.AliasUsage, [if_nested_deeper_than: 2, if_called_more_often_than: 1]}
       ]
     }
   ]

.github/workflows/lint.yml

@@ -76,3 +76,7 @@ jobs:
 
     - name: Compile the project
       run: mix compile
+
+    # TODO: We need to ignore issues we haven't addressed yet.
+    - name: Lint Elixir code to enforce code consistency # https://hexdocs.pm/credo/
+      run: mix credo --ignore Readability,Refactor

lib/teiserver/account.ex

@@ -2217,6 +2217,7 @@ defmodule Teiserver.Account do
   @spec update_client(T.userid(), map()) :: nil | :ok
   defdelegate update_client(userid, partial_client), to: ClientLib
 
+  # credo:disable-for-next-line Credo.Check.Design.TagTODO
   # TODO: Remove these in favour of update_client
   @spec merge_update_client(map()) :: nil | :ok
   defdelegate merge_update_client(client), to: ClientLib

lib/teiserver/account/reports/week_on_week_report.ex

@@ -43,12 +43,12 @@ defmodule Teiserver.Account.WeekOnWeekReport do
         previous_key = {week - 1, weekday}
         previous_value = data_map[previous_key]
 
-        if previous_value not in [nil, 0] do
+        if previous_value in [nil, 0] do
+          {key, nil}
+        else
           delta = NumberHelper.percent(this_value / previous_value) - 100
 
           {key, delta}
-        else
-          {key, nil}
         end
       end)
 
@@ -80,12 +80,12 @@ defmodule Teiserver.Account.WeekOnWeekReport do
         previous_key = key - 1
         previous_value = week_data[previous_key]
 
-        if previous_value not in [nil, 0] do
+        if previous_value in [nil, 0] do
+          {key, nil}
+        else
           delta = NumberHelper.percent(this_value / previous_value) - 100
 
           {key, delta}
-        else
-          {key, nil}
         end
       end)
 

lib/teiserver/account/schemas/user.ex

@@ -5,6 +5,7 @@ defmodule Teiserver.Account.User do
 
   alias Argon2
 
+  # credo:disable-for-next-line Credo.Check.Design.TagTODO
   # TODO: this is where a user should be defined. This is only a placeholder for now
   @type t :: term()
 

lib/teiserver/account/servers/login_throttle_server.ex

@@ -351,6 +351,7 @@ defmodule Teiserver.Account.LoginThrottleServer do
             {key, new_queue}
           end)
 
+        # credo:disable-for-next-line Credo.Check.Design.TagFIXME
         # FIXME: Waited for counter can be here with the now_ms value
         new_heartbeats = Map.drop(state.heartbeats, released_users)
 

lib/teiserver/autohost/tachyon_handler.ex

@@ -23,6 +23,7 @@ defmodule Teiserver.Autohost.TachyonHandler do
 
   @type start_response :: %{ips: [String.t()], port: integer()}
 
+  # credo:disable-for-next-line Credo.Check.Design.TagTODO
   # TODO: there should be some kind of retry here
   @spec start_battle(Bot.id(), Teiserver.TachyonBattle.id(), Teiserver.Autohost.start_script()) ::
           {:ok, start_response()} | {:error, term()}
@@ -167,6 +168,7 @@ defmodule Teiserver.Autohost.TachyonHandler do
            current_battles: current
          }) do
       {:error, {:already_registered, _pid}} ->
+        # credo:disable-for-next-line Credo.Check.Design.TagTODO
         # TODO: maybe we should handle that by disconnecting the existing one?
         {:stop, :normal, state}
 
@@ -227,6 +229,7 @@ defmodule Teiserver.Autohost.TachyonHandler do
 
   @impl true
   def handle_response("autohost/subscribeUpdates", _, _response, state) do
+    # credo:disable-for-next-line Credo.Check.Design.TagTODO
     # TODO: handle potential failure here
     # for example, autohost refuses any subscription with `since` older than 5 minutes
     {:ok, state}

lib/teiserver/battle.ex

@@ -468,6 +468,7 @@ defmodule Teiserver.Battle do
     already_finished? = match.finished != nil
     winning_ally_team = List.first(winning_ally_teams)
 
+    # credo:disable-for-next-line Credo.Check.Design.TagTODO
     # TODO Currently trusting the first received event, should be reworked to accept what the majority agrees on
     if winning_ally_team != nil && match.winning_team != nil &&
          winning_ally_team != match.winning_team do
@@ -619,6 +620,7 @@ defmodule Teiserver.Battle do
             # because the bot itself is in a new lobby since the last one finished
             script_tags = data["battleContext"]["scriptTags"]
 
+            # credo:disable-for-next-line Credo.Check.Design.TagTODO
             # TODO the server/match/id is legacy and should be removed
             # After updating Teiserver, there may be some ongoing matches with the legacy tags
             id = script_tags["game/server_match_id"] || script_tags["server/match/id"]

lib/teiserver/battle/balance/auto_balance.ex

@@ -87,7 +87,7 @@ defmodule Teiserver.Battle.Balance.AutoBalance do
   # If the top two players are in the same party, this will return true
   @spec has_op_party?([DB.player()]) :: boolean()
   def has_op_party?(players) do
-    if(Enum.count(players) >= 2) do
+    if Enum.count(players) >= 2 do
       sorted_players =
         Enum.sort_by(
           players,

lib/teiserver/battle/balance/brute_force.ex

@@ -132,7 +132,7 @@ defmodule Teiserver.Battle.Balance.BruteForce do
 
   @spec get_st_dev([BF.player()]) :: any()
   def get_st_dev(team) do
-    if(length(team) > 0) do
+    if length(team) > 0 do
       ratings = Enum.map(team, fn player -> player.rating end)
       Statistics.stdev(ratings)
     else
@@ -142,7 +142,7 @@ defmodule Teiserver.Battle.Balance.BruteForce do
 
   @spec get_captain_rating([BF.player()]) :: any()
   def get_captain_rating(team) do
-    if(length(team) > 0) do
+    if length(team) > 0 do
       captain = Enum.max_by(team, fn player -> player.rating end, &>=/2)
       captain.rating
     else