Add Credo checks and refactor code

Signed-off-by: Thomas Citharel <tcit@tcit.fr>

Make Logger.debug calls lazy

Signed-off-by: Thomas Citharel <tcit@tcit.fr>

Add missing @moduledocs

Signed-off-by: Thomas Citharel <tcit@tcit.fr>

Refactor according to credo

Signed-off-by: Thomas Citharel <tcit@tcit.fr>

Final fixes and add credo to CI

Signed-off-by: Thomas Citharel <tcit@tcit.fr>

Closes #52
This commit is contained in:
Thomas Citharel 2019-01-03 14:59:59 +01:00
parent ea82d392e9
commit 2f2c538cc9
No known key found for this signature in database
GPG key ID: A061B9DDE0CA0773
27 changed files with 163 additions and 121 deletions

View file

@ -21,8 +21,8 @@
# You can give explicit globs or simply directories.
# In the latter case `**/*.{ex,exs}` will be used.
#
included: ["lib/", "src/", "test/", "web/", "apps/"],
excluded: [~r"/_build/", ~r"/deps/"]
included: ["lib/", "src/", "test/"],
excluded: [~r"/_build/", ~r"/deps/", ~r"/js/"]
},
#
# If you create your own checks, you must specify the source files for
@ -51,12 +51,12 @@
#
## 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},
{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
@ -64,92 +64,91 @@
# You can customize the priority of any check
# Priority values are: `low, normal, high, higher`
#
{Credo.Check.Design.AliasUsage, priority: :low},
# For some checks, you can also set other parameters
#
# If you don't want the `setup` and `test` macro calls in ExUnit tests
# or the `schema` macro in Ecto schemas to trigger DuplicatedCode, just
# set the `excluded_macros` parameter to `[:schema, :setup, :test]`.
#
{Credo.Check.Design.DuplicatedCode, excluded_macros: []},
{Credo.Check.Design.AliasUsage,
[priority: :low, if_nested_deeper_than: 2, if_called_more_often_than: 0]},
# 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},
{Credo.Check.Design.TagFIXME},
{Credo.Check.Design.TagTODO, [exit_status: 0]},
{Credo.Check.Design.TagFIXME, []},
#
## Readability Checks
#
{Credo.Check.Readability.AliasOrder},
{Credo.Check.Readability.FunctionNames},
{Credo.Check.Readability.LargeNumbers},
{Credo.Check.Readability.MaxLineLength, priority: :low, max_length: 80},
{Credo.Check.Readability.ModuleAttributeNames},
{Credo.Check.Readability.ModuleDoc},
{Credo.Check.Readability.ModuleNames},
{Credo.Check.Readability.ParenthesesOnZeroArityDefs},
{Credo.Check.Readability.ParenthesesInCondition},
{Credo.Check.Readability.PredicateFunctionNames},
{Credo.Check.Readability.PreferImplicitTry},
{Credo.Check.Readability.RedundantBlankLines},
{Credo.Check.Readability.StringSigils},
{Credo.Check.Readability.TrailingBlankLine},
{Credo.Check.Readability.TrailingWhiteSpace},
{Credo.Check.Readability.VariableNames},
{Credo.Check.Readability.Semicolons},
{Credo.Check.Readability.SpaceAfterCommas},
{Credo.Check.Readability.AliasOrder, []},
{Credo.Check.Readability.FunctionNames, []},
{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.ParenthesesInCondition, []},
{Credo.Check.Readability.ParenthesesOnZeroArityDefs, []},
{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.VariableNames, []},
#
## Refactoring Opportunities
#
{Credo.Check.Refactor.DoubleBooleanNegation},
{Credo.Check.Refactor.CondStatements},
{Credo.Check.Refactor.CyclomaticComplexity},
{Credo.Check.Refactor.FunctionArity},
{Credo.Check.Refactor.LongQuoteBlocks},
{Credo.Check.Refactor.MatchInCondition},
{Credo.Check.Refactor.NegatedConditionsInUnless},
{Credo.Check.Refactor.NegatedConditionsWithElse},
{Credo.Check.Refactor.Nesting, max_nesting: 3},
{Credo.Check.Refactor.CondStatements, []},
{Credo.Check.Refactor.CyclomaticComplexity, []},
{Credo.Check.Refactor.FunctionArity, []},
{Credo.Check.Refactor.LongQuoteBlocks, []},
{Credo.Check.Refactor.MapInto, []},
{Credo.Check.Refactor.MatchInCondition, []},
{Credo.Check.Refactor.NegatedConditionsInUnless, []},
{Credo.Check.Refactor.NegatedConditionsWithElse, []},
{Credo.Check.Refactor.Nesting, [
max_nesting: 3
]},
{Credo.Check.Refactor.PipeChainStart,
excluded_argument_types: [:atom, :binary, :fn, :keyword], excluded_functions: []},
{Credo.Check.Refactor.UnlessWithElse},
[
excluded_argument_types: [:atom, :binary, :fn, :keyword, :number],
excluded_functions: [],
exit_status: 0
]},
{Credo.Check.Refactor.UnlessWithElse, []},
#
## Warnings
#
{Credo.Check.Warning.BoolOperationOnSameValues},
{Credo.Check.Warning.ExpensiveEmptyEnumCheck},
{Credo.Check.Warning.IExPry},
{Credo.Check.Warning.IoInspect},
{Credo.Check.Warning.LazyLogging},
{Credo.Check.Warning.OperationOnSameValues},
{Credo.Check.Warning.OperationWithConstantResult},
{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.RaiseInsideRescue},
{Credo.Check.Warning.BoolOperationOnSameValues, []},
{Credo.Check.Warning.ExpensiveEmptyEnumCheck, []},
{Credo.Check.Warning.IExPry, []},
{Credo.Check.Warning.IoInspect, []},
{Credo.Check.Warning.LazyLogging, []},
{Credo.Check.Warning.OperationOnSameValues, []},
{Credo.Check.Warning.OperationWithConstantResult, []},
{Credo.Check.Warning.RaiseInsideRescue, []},
{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, []},
#
# Controversial and experimental checks (opt-in, just remove `, false`)
#
{Credo.Check.Consistency.MultiAliasImportRequireUse, false},
{Credo.Check.Design.DuplicatedCode, false},
{Credo.Check.Readability.Specs, false},
{Credo.Check.Refactor.ABCSize, false},
{Credo.Check.Refactor.AppendSingleItem, false},
{Credo.Check.Refactor.DoubleBooleanNegation, false},
{Credo.Check.Refactor.VariableRebinding, false},
{Credo.Check.Warning.MapGetUnsafePass, false},
{Credo.Check.Consistency.MultiAliasImportRequireUse, false},
#
# Deprecated checks (these will be deleted after a grace period)
#
{Credo.Check.Readability.Specs, false}
{Credo.Check.Warning.UnsafeToAtom, false}
#
# Custom checks can be created using `mix credo.gen.check`.

View file

@ -36,12 +36,13 @@ js:
untracked: false
expire_in: 30 days
elixir_format:
elixir_check:
stage: back
before_script:
- mix deps.get
script:
- mix format --check-formatted --dry-run
- mix credo list
cache:
paths:
- deps

View file

@ -63,7 +63,7 @@ defmodule Mix.Tasks.Mobilizon.Instance do
will_overwrite = Enum.filter(paths, &File.exists?/1)
proceed? = Enum.empty?(will_overwrite) or Keyword.get(options, :force, false)
unless not proceed? do
if proceed? do
[domain, port | _] =
String.split(
Common.get_option(
@ -163,7 +163,7 @@ defmodule Mix.Tasks.Mobilizon.Instance do
else
Mix.shell().error(
"The task would have overwritten the following files:\n" <>
(Enum.map(will_overwrite, &"- #{&1}\n") |> Enum.join("")) <>
(will_overwrite |> Enum.map(&"- #{&1}\n") |> Enum.join("")) <>
"Rerun with `-f/--force` to overwrite them."
)
end

View file

@ -68,7 +68,7 @@ defmodule Mobilizon.Actors do
where: u.id == ^user.id
)
) do
nil -> get_actors_for_user(user) |> hd
nil -> user |> get_actors_for_user() |> hd
actor -> actor
end
end

View file

@ -1,4 +1,7 @@
defmodule Mobilizon.Actors.Service.Tools do
@moduledoc """
Common functions for actors services
"""
alias Mobilizon.Actors.User
@spec we_can_send_email(User.t(), atom()) :: :ok | {:error, :email_too_soon}

View file

@ -17,13 +17,18 @@ defmodule MobilizonWeb.API.Comments do
Creates a comment from an actor and a status
"""
@spec create_comment(String.t(), String.t(), String.t()) :: {:ok, Activity.t()} | any()
def create_comment(from_username, status, visibility \\ "public", inReplyToCommentURL \\ nil) do
def create_comment(
from_username,
status,
visibility \\ "public",
in_reply_to_comment_URL \\ nil
) do
with {:local_actor, %Actor{url: url} = actor} <-
{:local_actor, Actors.get_local_actor_by_name(from_username)},
status <- String.trim(status),
mentions <- Formatter.parse_mentions(status),
inReplyToComment <- get_in_reply_to_comment(inReplyToCommentURL),
{to, cc} <- to_for_actor_and_mentions(actor, mentions, inReplyToComment, visibility),
in_reply_to_comment <- get_in_reply_to_comment(in_reply_to_comment_URL),
{to, cc} <- to_for_actor_and_mentions(actor, mentions, in_reply_to_comment, visibility),
tags <- Formatter.parse_tags(status),
content_html <-
make_content_html(
@ -37,7 +42,7 @@ defmodule MobilizonWeb.API.Comments do
url,
to,
content_html,
inReplyToComment,
in_reply_to_comment,
tags,
cc
) do

View file

@ -1,4 +1,7 @@
defmodule MobilizonWeb.Context do
@moduledoc """
Guardian context for MobilizonWeb
"""
@behaviour Plug
import Plug.Conn

View file

@ -23,11 +23,9 @@ defmodule MobilizonWeb.ActivityPubController do
def actor(conn, %{"name" => name}) do
with %Actor{} = actor <- Actors.get_local_actor_by_name(name) do
cond do
conn |> get_req_header("accept") |> is_ap_header() ->
if conn |> get_req_header("accept") |> is_ap_header() do
conn |> render_ap_actor(actor)
true ->
else
conn
|> put_resp_content_type("text/html")
|> send_file(200, "priv/static/index.html")

View file

@ -1,4 +1,7 @@
defmodule MobilizonWeb.Resolvers.Category do
@moduledoc """
Handles the category-related GraphQL calls
"""
require Logger
alias Mobilizon.Actors.User

View file

@ -1,4 +1,7 @@
defmodule MobilizonWeb.Resolvers.Comment do
@moduledoc """
Handles the comment-related GraphQL calls
"""
require Logger
alias Mobilizon.Events.Comment
alias Mobilizon.Activity

View file

@ -1,4 +1,7 @@
defmodule MobilizonWeb.Resolvers.Event do
@moduledoc """
Handles the event-related GraphQL calls
"""
alias Mobilizon.Service.ActivityPub
alias Mobilizon.Activity
alias Mobilizon.Actors

View file

@ -1,4 +1,7 @@
defmodule MobilizonWeb.Resolvers.Group do
@moduledoc """
Handles the group-related GraphQL calls
"""
alias Mobilizon.Actors
alias Mobilizon.Actors.{Actor}
alias Mobilizon.Service.ActivityPub

View file

@ -1,4 +1,7 @@
defmodule MobilizonWeb.Resolvers.Person do
@moduledoc """
Handles the person-related GraphQL calls
"""
alias Mobilizon.Actors
alias Mobilizon.Service.ActivityPub

View file

@ -1,2 +0,0 @@
defmodule MobilizonWeb.Resolvers.Upload do
end

View file

@ -1,4 +1,7 @@
defmodule MobilizonWeb.Resolvers.User do
@moduledoc """
Handles the user-related GraphQL calls
"""
alias Mobilizon.Actors.{User, Actor}
alias Mobilizon.Actors
require Logger

View file

@ -1,4 +1,7 @@
defmodule MobilizonWeb.Schema do
@moduledoc """
GraphQL schema representation
"""
use Absinthe.Schema
import Absinthe.Resolution.Helpers, only: [dataloader: 1]

View file

@ -1,4 +1,7 @@
defmodule MobilizonWeb.UploadPlug do
@moduledoc """
Plug to intercept uploads
"""
use Plug.Builder
plug(Plug.Static,

View file

@ -1,4 +1,7 @@
defmodule MobilizonWeb.Uploaders.Avatar do
@moduledoc """
Handles avatar uploads
"""
use Arc.Definition
# Include ecto support (requires package arc_ecto installed):

View file

@ -1,4 +1,7 @@
defmodule MobilizonWeb.Uploaders.Category do
@moduledoc """
Handles file uploads for categories
"""
use Arc.Definition
use Arc.Ecto.Definition

View file

@ -43,14 +43,7 @@ defmodule Mobilizon.Service.ActivityPub do
def insert(map, local \\ true) when is_map(map) do
with map <- lazy_put_activity_defaults(map),
:ok <- insert_full_object(map) do
object_id =
cond do
is_map(map["object"]) ->
map["object"]["id"]
is_binary(map["object"]) ->
map["id"]
end
object_id = if is_map(map["object"]), do: map["object"]["id"], else: map["id"]
map = if local, do: Map.put(map, "id", "#{object_id}/activity"), else: map
@ -384,7 +377,7 @@ defmodule Mobilizon.Service.ActivityPub do
{:ok, data} = Transmogrifier.prepare_outgoing(activity.data)
json = Jason.encode!(data)
Logger.debug("Remote inboxes are : #{inspect(remote_inboxes)}")
Logger.debug(fn -> "Remote inboxes are : #{inspect(remote_inboxes)}" end)
Enum.each(remote_inboxes, fn inbox ->
Federator.enqueue(:publish_single_ap, %{

View file

@ -49,27 +49,34 @@ defmodule Mobilizon.Service.ActivityPub.Transmogrifier do
end
def fix_in_reply_to(%{"inReplyTo" => in_reply_to} = object)
when not is_nil(in_reply_to) do
in_reply_to_id =
cond do
# If the inReplyTo is just an AP ID
is_bitstring(in_reply_to) ->
in_reply_to
# If the inReplyTo is a object itself
is_map(in_reply_to) && is_bitstring(in_reply_to["id"]) ->
in_reply_to["id"]
# If the inReplyTo is an array
is_list(in_reply_to) && is_bitstring(Enum.at(in_reply_to, 0)) ->
Enum.at(in_reply_to, 0)
true ->
Logger.error("inReplyTo ID seem incorrect")
Logger.error(inspect(in_reply_to))
""
when not is_nil(in_reply_to) and is_bitstring(in_reply_to) do
in_reply_to |> do_fix_in_reply_to(object)
end
def fix_in_reply_to(%{"inReplyTo" => in_reply_to} = object)
when not is_nil(in_reply_to) and is_map(in_reply_to) do
if is_bitstring(in_reply_to["id"]) do
in_reply_to["id"] |> do_fix_in_reply_to(object)
end
end
def fix_in_reply_to(%{"inReplyTo" => in_reply_to} = object)
when not is_nil(in_reply_to) and is_list(in_reply_to) do
if is_bitstring(Enum.at(in_reply_to, 0)) do
in_reply_to |> Enum.at(0) |> do_fix_in_reply_to(object)
end
end
def fix_in_reply_to(%{"inReplyTo" => in_reply_to} = object)
when not is_nil(in_reply_to) do
Logger.error("inReplyTo ID seem incorrect")
Logger.error(inspect(in_reply_to))
do_fix_in_reply_to("", object)
end
def fix_in_reply_to(object), do: object
def do_fix_in_reply_to(in_reply_to_id, object) do
case fetch_obj_helper(in_reply_to_id) do
{:ok, replied_object} ->
object
@ -85,8 +92,6 @@ defmodule Mobilizon.Service.ActivityPub.Transmogrifier do
end
end
def fix_in_reply_to(object), do: object
def fix_attachments(object) do
attachments =
(object["attachment"] || [])
@ -500,7 +505,7 @@ defmodule Mobilizon.Service.ActivityPub.Transmogrifier do
@spec normalize(String.t()) :: struct() | nil
def get_anything_by_url(url) do
Logger.debug("Getting anything from url #{url}")
Logger.debug(fn -> "Getting anything from url #{url}" end)
get_actor_url(url) || get_event_url(url) || get_comment_url(url)
end

View file

@ -171,7 +171,7 @@ defmodule Mobilizon.Service.ActivityPub.Utils do
data =
if Map.has_key?(object_data, "inReplyTo") && object_data["inReplyTo"] != nil &&
object_data["inReplyTo"] != "" do
Logger.debug("Object has inReplyTo #{object_data["inReplyTo"]}")
Logger.debug(fn -> "Object has inReplyTo #{object_data["inReplyTo"]}" end)
case ActivityPub.fetch_object_from_url(object_data["inReplyTo"]) do
# Reply to an event (Comment)

View file

@ -4,6 +4,9 @@
# Upstream: https://git.pleroma.social/pleroma/pleroma/blob/develop/lib/pleroma/formatter.ex
defmodule Mobilizon.Service.Formatter do
@moduledoc """
Formats input text to structured data, extracts mentions and hashtags
"""
alias Mobilizon.Actors.Actor
alias Mobilizon.Actors

View file

@ -96,7 +96,8 @@ defmodule Mobilizon.Mixfile do
{:mix_test_watch, "~> 0.5", only: :dev, runtime: false},
{:ex_unit_notifier, "~> 0.1", only: :test},
{:dialyxir, "~> 1.0.0-rc.4", only: [:dev], runtime: false},
{:exvcr, "~> 0.10", only: :test}
{:exvcr, "~> 0.10", only: :test},
{:credo, "~> 1.0.0", only: [:dev, :test], runtime: false}
]
end

View file

@ -17,7 +17,7 @@
"cors_plug": {:hex, :cors_plug, "1.5.2", "72df63c87e4f94112f458ce9d25800900cc88608c1078f0e4faddf20933eda6e", [:mix], [{:plug, "~> 1.3 or ~> 1.4 or ~> 1.5", [hex: :plug, repo: "hexpm", optional: false]}], "hexpm"},
"cowboy": {:hex, :cowboy, "1.1.2", "61ac29ea970389a88eca5a65601460162d370a70018afe6f949a29dca91f3bb0", [:rebar3], [{:cowlib, "~> 1.0.2", [hex: :cowlib, repo: "hexpm", optional: false]}, {:ranch, "~> 1.3.2", [hex: :ranch, repo: "hexpm", optional: false]}], "hexpm"},
"cowlib": {:hex, :cowlib, "1.0.2", "9d769a1d062c9c3ac753096f868ca121e2730b9a377de23dec0f7e08b1df84ee", [:make], [], "hexpm"},
"credo": {:hex, :credo, "0.9.3", "76fa3e9e497ab282e0cf64b98a624aa11da702854c52c82db1bf24e54ab7c97a", [:mix], [{:bunt, "~> 0.2.0", [hex: :bunt, repo: "hexpm", optional: false]}, {:poison, ">= 0.0.0", [hex: :poison, repo: "hexpm", optional: false]}], "hexpm"},
"credo": {:hex, :credo, "1.0.0", "aaa40fdd0543a0cf8080e8c5949d8c25f0a24e4fc8c1d83d06c388f5e5e0ea42", [:mix], [{:bunt, "~> 0.2.0", [hex: :bunt, repo: "hexpm", optional: false]}, {:jason, "~> 1.0", [hex: :jason, repo: "hexpm", optional: false]}], "hexpm"},
"dataloader": {:hex, :dataloader, "1.0.4", "7c2345c53c9e5b61420013fc53c8463ba347a938b61f66677eb47d9c4a53ac5d", [:mix], [{:ecto, ">= 0.0.0", [hex: :ecto, repo: "hexpm", optional: true]}], "hexpm"},
"db_connection": {:hex, :db_connection, "1.1.3", "89b30ca1ef0a3b469b1c779579590688561d586694a3ce8792985d4d7e575a61", [:mix], [{:connection, "~> 1.0.2", [hex: :connection, repo: "hexpm", optional: false]}, {:poolboy, "~> 1.5", [hex: :poolboy, repo: "hexpm", optional: true]}, {:sbroker, "~> 1.0", [hex: :sbroker, repo: "hexpm", optional: true]}], "hexpm"},
"decimal": {:hex, :decimal, "1.6.0", "bfd84d90ff966e1f5d4370bdd3943432d8f65f07d3bab48001aebd7030590dcc", [:mix], [], "hexpm"},

View file

@ -228,7 +228,7 @@ defmodule Mobilizon.ActorsTest do
@remote_actor_key {:ok,
{:RSAPublicKey,
20_890_513_599_005_517_665_557_846_902_571_022_168_782_075_040_010_449_365_706_450_877_170_130_373_892_202_874_869_873_999_284_399_697_282_332_064_948_148_602_583_340_776_692_090_472_558_740_998_357_203_838_580_321_412_679_020_304_645_826_371_196_718_081_108_049_114_160_630_664_514_340_729_769_453_281_682_773_898_619_827_376_232_969_899_348_462_205_389_310_883_299_183_817_817_999_273_916_446_620_095_414_233_374_619_948_098_516_821_650_069_821_783_810_210_582_035_456_563_335_930_330_252_551_528_035_801_173_640_288_329_718_719_895_926_309_416_142_129_926_226_047_930_429_802_084_560_488_897_717_417_403_272_782_469_039_131_379_953_278_833_320_195_233_761_955_815_307_522_871_787_339_192_744_439_894_317_730_207_141_881_699_363_391_788_150_650_217_284_777_541_358_381_165_360_697_136_307_663_640_904_621_178_632_289_787,
65537}}
65_537}}
test "test get_public_key_for_url/1 with remote actor" do
use_cassette "actors/remote_actor_mastodon_tcit" do
assert Actor.get_public_key_for_url(@remote_account_url) == @remote_actor_key

View file

@ -1,4 +1,7 @@
defmodule MobilizonWeb.AbsintheHelpers do
@moduledoc """
Absinthe helpers for tests
"""
def query_skeleton(query, query_name) do
%{
"operationName" => "#{query_name}",