In case function has no docs, use the ones from callback

[michalmuskala]

As discussed over on IRC. A function that is not explicitly annotated with @doc false and doesn't have docs, but the callback does - will include callback docs.

[elixir-bot]

Hello, @michalmuskala! This is your first Pull Request that will be reviewed by Ebert, an automatic Code Review service. It will leave comments on this diff with potential issues and style violations found in the code as you push new commits. You can also see all the issues found on this Pull Request on its review page. Please check our documentation for more information.

[elixir-bot]

The condition of if should not be wrapped in parentheses.

[whatyouhide]

I already don't like you @elixir-bot 😄

[josevalim]

We have already reported this to Credo, it should be fixed soon. :)

[josevalim]

What if the callback module was compiled without docs? It can be in a separate project.

I would do:

callback_docs = Code.get_docs(behaviour, :callback_docs) || []
case List.keyfind(callback_docs, {name, arity}, 0) do
  {{^name, ^arity}, _, :callback, callback_docs} when is_binary(callback_docs) -> ...

[josevalim]

I think it would be better to list this at the end, no? The question is: what do we want the summary of this function to be? I think we want to keep the callback original summary. If so, this should go to the bottom.

[josevalim]

@michalmuskala beautiful, I have added some comments.

[josevalim]

Also, when we merge this, let's please do the same for IEx.Helpers.h. :)

[michalmuskala]

@josevalim I've updated the code. I decided to go for a solution where we extract the first line from the behaviour docs and keep it as the first line for the summary, injecting an additional paragraph at the beginning informing the user that it's not the documentation for the function itself, but rather the callback.

[michalmuskala]

Also, it seems the h helper already has something similar: https://github.com/elixir-lang/elixir/blob/master/lib/iex/test/iex/helpers_test.exs#L76-L108

[josevalim]

@michalmuskala I would say let's keep it simple, because well, it is simpler. For example, the current code will crash if the docs contain no new lines, such as @doc "foo" and it is splitting on new lines while it should be on \n\n. None of those would matter if we always add the note to the end, which I believe is better and simpler.

[josevalim]

@michalmuskala cool, so I think for h we just need to add a notice to the end that says "Implementation for callback x.y/z"?

[michalmuskala]

@josevalim I believe h will print the @callback foo(...) banner instead of def foo(...) when printing callback docs, so we may be fine.

[josevalim]

@michalmuskala good call, that part is good then!

[josevalim]

❤️ 💚 💙 💛 💜

[wojtekmach]

@michalmuskala I'm curious if you run into this issue when browsing Ecto docs. I noticed that https://hexdocs.pm/ecto/Ecto.Multi.html#insert_all/5 isn't correctly autolinking to Repo docs (even after checking on ex_doc master with ecto docs fix (elixir-ecto/ecto#1815)).

Perhaps it's a different issue though, but I figured it's worth asking before I look into it more

[josevalim]

@wojtekmach it should be c:Ecto.Repo.insert_all/4 so it is a bug in Ecto's documentation. :)

[wojtekmach]

doh, that makes sense. I will open up another doc fix on Ecto :)

2016-11-21 17:06 GMT+01:00 José Valim notifications@github.com:

@wojtekmach https://github.com/wojtekmach it should be
c:Ecto.Repo.insert_all/4 so it is a bug in Ecto's documentation. :)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#641 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAEpJ1qiqrhTXunlOXRbSrfu8Wayrivoks5rAcGIgaJpZM4Kxim_
.

Wojtek Mach

[wojtekmach]

@josevalim that was it, thanks! Btw, is it expected that iex shows c: prefix as on screenshot below?

diff --git a/lib/ecto/multi.ex b/lib/ecto/multi.ex
index dce9cbf..135f7eb 100644
--- a/lib/ecto/multi.ex
+++ b/lib/ecto/multi.ex
@@ -301,7 +301,7 @@ defmodule Ecto.Multi do
   @doc """
   Adds an insert_all operation to the multi.

-  Accepts the same arguments and options as `Ecto.Repo.insert_all/3` does.
+  Accepts the same arguments and options as `c:Ecto.Repo.insert_all/3` does.
   """
   @spec insert_all(t, name, schema_or_source, [entry], Keyword.t) :: t
         when schema_or_source: binary | {binary | nil, binary} | Ecto.Schema.t,

or maybe IEx should be taught to parse this and leave it out?

[josevalim]

I would leave it in because otherwise you may try to do h Ecto.Repo.insert_all/3 and it won't work.