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

Behaviour docs should be stored in the byte code too #3088

Closed
josevalim opened this issue Feb 15, 2015 · 9 comments · Fixed by #3169
Closed

Behaviour docs should be stored in the byte code too #3088

josevalim opened this issue Feb 15, 2015 · 9 comments · Fixed by #3169

Comments

@josevalim
Copy link
Member

No description provided.

@lexmag
Copy link
Member

lexmag commented Mar 9, 2015

I’d like to give it a shot.

@josevalim
Copy link
Member Author

@lexmag awesome! Go ahead and let me know if you have any questions.

@lexmag
Copy link
Member

lexmag commented Mar 10, 2015

I think there are 2 possible places to store behaviour docs before adding them to beam chunk:

  • use same ets docs entry for it as all other docs do (except module doc), thus we can reuse Module.add_doc/6 function by just adding :defcallback and :defmacrocallback to the kinds list; it makes possible to use h helper to get docs for the callbacks
  • (lexmag@5622c85) introduce new entry (behaviour_docs?) and stricter separation; this requires elixir_docs_v2 in elixir_module:add_docs_chunk/4 and new helper for retrieving docs in the IEx

Personally, I like the first approach but not sure how it'll work with overlapped functions, e.g. Dict module has same definitions for behaviour callbacks and for interface functions.

@josevalim what are your thoughts on it?

@josevalim
Copy link
Member Author

@lexmag very nice!

I am not sure which one we should go either. @ericmj, @fishcakez and @alco, thoughts?

Btw, if we decide to go with 2, I don't think we need elixir_docs_v2 because it is a keyword list and we are keeping it as such.

@lexmag
Copy link
Member

lexmag commented Mar 10, 2015

In the meantime, while waiting for feedback, let’s fix the warnings in master. 😄

lib/keyword.ex:1: warning: undefined behaviour function get_lazy/3 (for behaviour Dict)
lib/keyword.ex:1: warning: undefined behaviour function pop_lazy/3 (for behaviour Dict)
lib/keyword.ex:1: warning: undefined behaviour function put_new_lazy/3 (for behaviour Dict)

Can't we use use Dict instead of @behaviour Dict there?

@josevalim
Copy link
Member Author

@lexmag let's implement manually because the implementation will certainly be faster than the one in Dict (since we can specialize it here). Please do send a PR!

@fishcakez
Copy link
Member

I don't like the idea of h Module.callback/2 showing the callback information because a callback is not "callable" like that. However at the same time I really like the idea of being able to do h .. to get the specific information. www.erlang.org/erldoc uses this format when searching.

@josevalim
Copy link
Member Author

After @fishcakez comment, I have realized that 1) is not backwards compatible. We are adding entries to the list which are not "callables". This will definitely break IEx autocomplete and likely other APIs. So let's go with 2.

@lexmag
Copy link
Member

lexmag commented Mar 10, 2015

Feels reasonable. Will continue with the 2 option then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

3 participants