Hi
When I use @behaviour
and @callback
, the functions are defined. I guess that I do not need to use @spec
for the implementation of the @impl
functions as well?
Will Dialyzer sort this out as well?
Br Patrik
Hi
When I use @behaviour
and @callback
, the functions are defined. I guess that I do not need to use @spec
for the implementation of the @impl
functions as well?
Will Dialyzer sort this out as well?
Br Patrik
You don’t need them as Dialyzer will give a callback does not match spec error, but please include them anyway. As a reader I do not want to have to jump to the behaviour definition to find out what the arguments are. In that vein if you include a using macro where you define default callbacks that are overridable with defoverridable, elide the spec in the using macro, otherwise you’ll get a compiler error for duplicating the spec if anyone actually overrides and wants to spec the override.
YES
defmodule MyBehaviour do
@callback foo() :: :ok
end
defmodule MyImpl do
@impl MyBehaviour
@spec foo() :: :ok
def foo(), do: :ok
end
YES
defmodule MyBehaviour do
@callback foo() :: :ok
defmacro __using__(_) do
quote do
@impl MyBehaviour
@spec foo() :: :ok
def foo(), do: :ok
end
end
end
defmodule MyImpl do
use MyBehaviour
end
YES
defmodule MyBehaviour do
@callback foo() :: :ok
defmacro __using__(_) do
quote do
@impl MyBehaviour
def foo(), do: :ok
defoverridable [foo: 0]
end
end
end
defmodule MyImpl do
use MyBehaviour
@impl MyBehaviour
@spec foo() :: :ok
def foo(), do: :ok
end
NO
defmodule MyBehaviour do
@callback foo() :: :ok
defmacro __using__(_) do
quote do
@impl MyBehaviour
@spec foo() :: :ok
def foo(), do: :ok
defoverridable [foo: 0]
end
end
end
defmodule MyImpl do
use MyBehaviour
# this errors if you include the spec
# @spec foo() :: :ok
@impl MyBehaviour
def foo(), do: :ok
end
I don’t like such ideas as it’s really a pain for maintainers of libraries in case multiple libraries are implementing specified behaviour. Imagine that one behaviour is used by 10 libraries and 3 of them are not updated - what to do in such case? Should someone fork it just to update @docs
?
Personalyl I think that introducing macro for documentation is a bit too overcomplicated. In such cases I would prefer to use h
helper in iex
(or just html
documentation) and docs delegating feature:
@doc delegate_to: {Foo, :bar, 3}
This would add a link which could be used in h
helper (or just clicked on html
page). In such case there is no need to change same documentation for multiple implementations or write any macros.
I’m very unclear which part of my suggestion you’re taking umbrage with. Can you give an example of what you’re talking about?
Sure, here is your changed code:
defmodule MyBehaviour do
@callback foo() :: :ok
end
defmodule MyImpl do
@behaviour MyBehaviour
@doc delegate_to: {MyBehaviour, :foo, 0}
@doc "Implementation-specific docs goes here …"
@impl MyBehaviour
def foo(), do: :ok
end
which would give:
iex(1)> h MyImpl.foo
def foo()
delegate_to: MyBehaviour.foo/0
Implementation-specific docs goes here …
iex(2)> b MyBehaviour.foo/0
@callback foo() :: :ok
This is much simpler than writing macros or copy-paste documentation and spec.
Generally we should avoid using macros unless it’s required.
I wasn’t suggesting to use a macro. Simply saying that if you do have a macro and the function is overridable to elide the spec in the macro (as they do in e.g. GenServer but do NOT elide in HTTPoison). I don’t think they’re incompatible, unless I’m misunderstanding something. This thread is about @spec
not @doc
.
Yes, it is - look that delegated callback/function have also its specification (not only documentation - I did not even added documentation to your behaviour code).
As said if there is really no need to write macro then we should avoid doing it + it makes exactly no sense to copy @doc
and @spec
over all implementations (especially manually i.e. without macro).
Simply look how much code we wrote + how much nested spaces you have in macro which is not needed in such case.
If we want to tell that some documentation and/or specification is same for our function then we can simply delegate it in @doc
which would give others enough information.
Again, I’m not advocating for the macro, at all, in any way shape or form. I agree in this case it’s not needed. I’m contrasting the approaches taken between GenServer
and HTTPoison.Base
.
vs.
Dialyzer will pick this all up even if you drop all the @specs
. But for readers of the code, giving them the ability to @spec
the implementation is much more pleasant (even if they choose not to), because they do not need to look outside the file or inside IEx to be able to figure out what’s going on. And if they have the Credo rule on to require @spec
for all functions, you must ignore in the implementation because the compiler will complain about duplicate specs for the implemented callbacks.
GenServer
is not good example here is it’s most probably intended to not add @doc
and @spec
to those functions. Anyway I can see what you are talking about. For sure adding @doc false
when you really expect documentation is really bad, but @doc delegate_to
is still ok here
More … for this use case when most of projects don’t even document those functions it could be nice to properly link them to GenServer
documentation page. I believe that core team does not wanted to add extra documentation for every module which uses GenServer as reading documentation could be a bit harder due to more documentation.
I have similar feeling to HTTPoison
. Omitting that you are linking to deprecated function we can still use @doc delegate_to
in such case without any problem.
ok, so for me it’s even not an option to consider
Personally I don’t like be forced to something. I’m like Erlang/Elixir
- I can fail as much as I need. No matter how much times - sooner or later I would be better. If I would be limited in order to protect myself then I’m not going to make fails and learn on them. Look that Elixir
is written to be as much extensible as possible.
The goal here is to introduce well known standards (just like adding optional @spec
support), but not force them. Imagine what would happen if suddenly all hex
libraries would fail, because @spec
would be required for all functions. Look that @spec
everywhere would be like a dream for readers, but also huge pain for maintainers.
Sooner or later you would get an edge-case. There is no rule in world to cover all cases, so forcing anything is never a good idea. It’s why phoenix
is not called a framework
, but library
.
You have lots of cases when you need to take a look at other modules to understand code properly especially in cases like GenServer
. You just need to remind from time to time handle_call
, handle_cast
and handle_info
.
Personally I think that delegating documentation is much better, because same documentation and spec does not need to be written multiple times. Of course we do not see it in such simple examples.
Simply compare:
@doc delegate_to: {MyBehaviour, :foo, 0}
which is never going to change with copy-paste long specifications especially with map (optional and required keys).
There is no even need to imagine long map specification. Just look at really simple init/1
specification:
You would have few extra lines for each implementation’s function just to not make one click on HTML
page and it’s not even middle size of typical real world specification…
Hi
Wow, thanks for all comments! Did not know that this would stir up this many opinions.
A see your point @asummers, but since this is not a public library (“only” used within our company), I prefer that the implementers spend the extra time to go into the definition of the behaviour rather than using multiple specs that will effect the maintenance in the long run.
I also found that the @spec
may state less than the actual @callback
without Dialyzer telling me, witch gives me another argument not to use the @specs…
Example given:
defmodule Register.DocEvents do
@callback initialize(soure :: binary() | atom()) :: :ok | {:error, String.t()}
end
defmodule Register do
@behaviour Register.DocEvents
@impl Register.DocEvents
@spec initialize(atom()) :: :ok
def initialize(source) do
...
end
end
Dialyzer signals this is ok, and I guess it is since the actual implementation fits within the original specification. In this case, the @spec
makes sense since this implementation is not the same as the @callback
stated, but if they are expected to be the same then adding an extra @spec
just creates more maintenance burden.
For sure if you have different @spec
for specific implementation than @callback
then you should use @spec
. It’s important to let other knows that you will not meet exactly all cases expected by @behaviour
. Otherwise I suggest delegating to callback documentation as I have mentioned previously. For dialyzer
it’s ok probably because both arguments and return value match @callback
specification - only small part, but matches.
Hi, I want to use @doc delegate_to:
of a macro, but it doesn’t show me the doc I need
defmodule MishkaDatabase.CRUD
@doc """
### Creating a record macro
"""
defmacro crud_add(attrs)
...
end
end
delegate doc
:defmodule MishkaDatabase.Public.Setting do
@behaviour MishkaDatabase.CRUD
@doc delegate_to: {MishkaDatabase.CRUD, :crud_add, 1}
def create(attrs) do
crud_add(attrs)
end
end
when I run h
, it just shows me @callback
like this, not the doc I need
iex(25)> h MishkaDatabase.Public.Setting.create
@callback create(record_input()) ::
{:error, :add, error_tag(), repo_error()}
| {:ok, :add, error_tag(), repo_data()}
Looks like a bug. When @callback
is used delegate_to
option seems to be ignored …
As far as I know we do not have any copy-paste documentation mechanism in Elixir
which is good as you may want to have it’s own custom documentation here and for further details you should see delegated documentation …
If it works like this, we can just put an empty @doc ""
to fix the bug concerned, but this feature even is not linked on document after generating just we can see the link on iex
Am I right?
Honestly I do not remember … However what you wrote is no longer an Elixir
core part, but ex_doc
library.
I have created bug report:
Thank you for the discussion everyone!
it helped me really to read the different perspectives and make decisions.