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

Adopt EEP 48 #7198

Closed
josevalim opened this Issue Jan 11, 2018 · 8 comments

Comments

Projects
None yet
3 participants
@josevalim
Copy link
Member

josevalim commented Jan 11, 2018

http://erlang.org/eep/eeps/eep-0048.html

The goal is to introduce Code.fetch_docs/1 that returns the format in the EEP (or {:error, _} if the chunk is not available). Since the old documentation chunk will no longer exist, Code.get_docs/2 should be changed to always return nil - which is a backwards compatible result - and be scheduled for deprecation.

We will also use the new chunk metadata to store relevant information, such as "defaults", "deprecated" and "since".

Replaces #3589.

@lackac

This comment has been minimized.

Copy link
Contributor

lackac commented Jun 22, 2018

@josevalim this sounds like something interesting to work on as it touches multiple areas including some erlang code. I would like to take a stab at it, but I need some guidance.

As far as I can tell completing this would involve the following tasks:

  • change or extend docs_chunk/6 to generate the new Docs chunk according to EEP-48
  • read the new chunk in Code.fetch_docs/1
  • adjust Protocol and escript.build mix task to work with the new chunk
  • adjust Code.get_docs/2 to work from the return value of Code.fetch_docs/1 – see question 3) below

Questions

  1. Should the existing behaviour be kept (also including the ExDc chunk, and keeping the functionality of Code.get_docs/2)? You mention to only return nil, which would mean no to both, but that will potentially break tools that rely on them. On the other hand keeping them would mean larger .beam files with both chunks present.
  2. Is including more chunk metadata in the scope of this issue? If so, should we also remove the ExDp chunk?
  3. The issue description suggests Code.fetch_docs/1 to "return the format in the EEP". This sounds like we want to return the term stored in the chunk unchanged. Since the EEP-48 structure is different from the current ExDc the tooling around it will need some adjustments. Am I right that the tooling mostly uses Code.get_docs/2 currently? If so, instead of changing it to return nil wouldn't it be better to keep the interface but use Code.fetch_docs/1 inside?

What did I miss?

@josevalim

This comment has been minimized.

Copy link
Member

josevalim commented Jun 22, 2018

@lackac that's a perfect summary. I also think we can do it in separate PRs. For example: first add the new chunk and remove related functionality, only then remove the old ones.

Answers:

  1. Code.get_docs will always return nil, forever. The ExDc will be completely removed.

  2. We can tackle Since and Deprecated as a separate issue. But ExDp won't be removed as it exists for other purposes.

  3. I want to perform a "clean break". Old tooling will have to be updated to use Code.fetch_docs since Code.get_docs always return nil. So we won't perform any code of conversion between old and new chunks.

@lackac

This comment has been minimized.

Copy link
Contributor

lackac commented Jun 22, 2018

@josevalim that makes sense. So to clarify, first PR adds the new Docs chunk, keeps ExDc in place and changes Code.get_docs/2 to return nil. Second PR would remove the ExDc chunk.

  1. Good to know.
  2. This also means that changing the tooling that's part of Elixir should be in scope for this PR. Why did you decide to change the semantics of Code.fetch_docs/1 compared to Code.get_docs/2? Would it be the responsibility of the caller to parse out the relevant information from the chunk (e.g. the specific function documentation)? Shouldn't we also provide some helper functions to do part of that work similarly to what the second argument of Code.get_docs/2 does (e.g. Code.get_function_docs(module, function, arity))?
@lackac

This comment has been minimized.

Copy link
Contributor

lackac commented Jun 22, 2018

related question, are you aware of an work on providing the Docs chunks on the Erlang side?

@josevalim

This comment has been minimized.

Copy link
Member

josevalim commented Jun 22, 2018

@lackac in order to break the work apart, we can change Code.get_docs only when we remove ExDc. :) So in general lines:

  1. Add new chunk
  2. Change related functionality to use new chunk
  3. Remove old chunk and return nil from Code.get_docs

Would it be the responsibility of the caller to parse out the relevant information from the chunk (e.g. the specific function documentation)?

Yes, it will be the caller responsibility. Maybe we will add helpers in the future but now it is too early to tell.

@josevalim

This comment has been minimized.

Copy link
Member

josevalim commented Jun 22, 2018

related question, are you aware of an work on providing the Docs chunks on the Erlang side?

@erszcz may have started on it or at least have some thoughts.

@lackac

This comment has been minimized.

Copy link
Contributor

lackac commented Jun 22, 2018

@erszcz

This comment has been minimized.

Copy link

erszcz commented Jun 22, 2018

Indeed, I have a partial implementation on https://github.com/erszcz/docsh/tree/eep-48. I'd be happy to do some interop testing once we're close enough to completion with both implementations.

One thing worth paying special attention to is mime_type() which is not defined in EEP-48 (and I'm not aware of its definition anywhere in Erlang documentation, but correct me, please, if I'm wrong on that). I've assumed it to be just a binary() with the IANA registered media type, which in case of Elixir would be text/markdown. Alas, EDoc is not registered by IANA. I'm also not sure what format the OTP team might target if/when they decide to generate docs_v1 chunks from the OTP SGML documentation.

This was referenced Jul 2, 2018

@josevalim josevalim closed this Jul 10, 2018

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