Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix dialyzer #2

Closed
wants to merge 14 commits into from
Closed

Fix dialyzer #2

wants to merge 14 commits into from

Conversation

alanvardy
Copy link
Contributor

@alanvardy alanvardy commented Feb 9, 2020

Round 2!

I was having trouble integrating your library with my Dialyzer specs, and have some dialyzer fixes for you!

I use a lot of tooling, including a formatter, and did my best to hold it back from formatting your whole code base with mixed success. Let me know what I can revert!

Before:

Total errors: 12, Skipped: 0, Unnecessary Skips: 0
done in 0m1.35s
lib/actions.ex:40:invalid_contract
The @spec for the function does not match the success typing of the function.

Function:
EctoShorts.Actions.all/2

Success typing:
@spec all(
  %Ecto.Query{
    :aliases => _,

...etc...

________________________________________________________________________________
done (warnings were emitted)
Halting VM with exit status 2

After:

Total errors: 0, Skipped: 0, Unnecessary Skips: 0
done in 0m1.45s
done (passed successfully)

@alanvardy alanvardy requested a review from MikaAK February 9, 2020 20:13
@alanvardy
Copy link
Contributor Author

Oh! and attempting to call Actions.get(ModuleName, id) still fails dialyzer, I am hoping to write a few tests for that in the next round.

@type query :: Ecto.Query | Ecto.Schema
@type filter_params :: Keyword.t | map
@type queryable :: module | Ecto.Query.t()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of the issues were around using Ecto.Schema or Ecto.Schema.t() when the spec needed was module (atom)

@type aggregate_options :: :avg | :count | :max | :min | :sum
@type schema_list :: list(Ecto.Schema.t) | []
@type schema_res :: {:ok, Ecto.Schema.t} | {:error, String.t}
@type schema_res :: {:ok, Ecto.Schema.t()} | {:error, String.t()}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, my formatter is going buck wild on these brackets. Let me know if this is an issue and I will get it reverted.

@spec all(queryable :: query) :: schema_list
@spec all(queryable) :: [Ecto.Schema.t()]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was totally unnecessary on my part.

Comment on lines -93 to +98
def find_or_create(schema, params) do
with {:error, %{code: :not_found}} <- find(schema, params) do
def find_or_create(query, params) do
with {:error, _} <- find(query, params) do
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this was the right move here. It didn't want to pattern match according to dialyzer. Are there other error codes?

@spec preload_changeset_assoc(Changeset.t, atom, list(integer)) :: Changeset.t
@spec preload_changeset_assoc(Changeset.t, atom, keyword) :: Changeset.t
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this was a good catch :)

is_nil(params_data) -> cast_assoc(changeset, key, opts)
Enum.empty?(params_data) ->
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is another good one. I believe this can only be a list and cannot be nil

Comment on lines -15 to 18
@spec query_schema(Ecto.Query.t) :: Ecto.Schema.t
@spec query_schema(Ecto.Query.t) :: module
@doc "Pulls the schema from a query"
def query_schema(%{from: %{source: {_, schema}}}), do: query_schema(schema)
def query_schema(%{from: %{query: %{from: {_, schema}}}}), do: schema
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returns a module name (atom) not a struct

Comment on lines 1 to 40
defmodule EctoShorts.Repo do
@moduledoc "This module is responsible for calling repo from config"
@type queryable :: module | Ecto.Query.t()
@type aggregates :: :avg | :count | :max | :min | :sum
@type structs_or_struct_or_nil :: [Ecto.Schema.t()] | Ecto.Schema.t() | nil

def call(func, args) do
@spec insert(Ecto.Changeset.t()) :: {:error, Ecto.Changeset.t()} | {:ok, Ecto.Schema.t()}
def insert(changeset), do: call(:insert, [changeset])

@spec delete(Ecto.Schema.t()) :: {:error, Ecto.Changeset.t()} | {:ok, Ecto.Schema.t()}
def delete(struct), do: call(:delete, [struct])

@spec update(Ecto.Changeset.t()) :: {:error, Ecto.Changeset.t()} | {:ok, Ecto.Schema.t()}
def update(changeset), do: call(:update, [changeset])

@spec get(queryable, integer) :: nil | Ecto.Schema.t()
def get(module, id), do: call(:get, [module, id])

@spec one(Ecto.Query.t()) :: nil | Ecto.Schema.t()
def one(query), do: call(:one, [query])

@spec all(queryable) :: [Ecto.Schema.t()]
def all(query), do: call(:all, [query])

@spec preload(structs_or_struct_or_nil, any, keyword) :: structs_or_struct_or_nil
def preload(structs_or_struct_or_nil, preloads, opts) do
call(:preload, [structs_or_struct_or_nil, preloads, opts])
end

@spec aggregate(Ecto.Queryable.t(), aggregates, atom, keyword) :: any | nil
def aggregate(queryable, aggregate, field, opts),
do: call(:aggregate, [queryable, aggregate, field, opts])

@spec stream(Ecto.Query.t()) :: Enum.t()
def stream(query), do: call(:stream, [query])

defp call(func, args) do
apply(repo(), func, args)
end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know whether this was really needed, I was having trouble tracing all the possibilities and I think that dialyzer just had too many possible inputs and outputs here. I separated them out and a lot of difficulties evaporated. There shouldn't be any actual change in your library's logic.

@alanvardy
Copy link
Contributor Author

Closing for now, but would be willing to take another crack at this in the future 👍

@alanvardy alanvardy closed this Aug 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant