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

Refactor fetching callbacks #1329

Merged
merged 3 commits into from
Feb 5, 2021
Merged

Refactor fetching callbacks #1329

merged 3 commits into from
Feb 5, 2021

Conversation

wojtekmach
Copy link
Member

No description provided.

test/test_helper.exs Outdated Show resolved Hide resolved

{:type, anno, _, _} = hd(specs)

Choose a reason for hiding this comment

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

I have a silly question about the best way to use and references when using Kernel functions... so my question is it's okay to call only the function and avoid doing a full call like Kernel.hd(specs)?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, it's totally ok to skip Kernel. since it's automatically imported everywhere.

Choose a reason for hiding this comment

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

so the only reason to use a full declaration is when using pipes

Copy link
Member Author

Choose a reason for hiding this comment

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

This is unrelated to pipes, you'd use full declaration when you're calling function from another module. You can skip the module name if the function is local to the current module or was imported using import/2. Kernel and Kernel.SpecialForms are automatically imported to every module, that's why you can call e.g. hd(list) instead of Kernel.hd(list).


:error ->
# gen_statem:state_name/3 is stored as StateName/3 in the docs chunk
name = name |> Atom.to_string() |> Macro.underscore() |> String.to_atom()
Copy link
Member

Choose a reason for hiding this comment

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

Is this a bug on the chunk?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is complicated.

We have:

iex> :gen_statem.behaviour_info(:callbacks)
[
  state_name: 3,
  ...
]

And:

Code.fetch_docs(:gen_statem) |> elem(6) |> Enum.map(&elem(&1, 0)) |> Enum.filter(&elem(&1, 0) == :callback)
[
  {:callback, :StateName, 3},
  # ...
]

And:

iex> b :gen_statem
@callback state_name(:enter, oldStateName :: state_name(), data :: data()) ::
            state_enter_result(:state_name)
@callback state_name(event_type(), eventContent :: term(), data :: data()) ::
            event_handler_result(state_name())

# ...

And:

b :gen_statem.state_name
@callback state_name(:enter, oldStateName :: state_name(), data :: data()) ::
            state_enter_result(:state_name)
@callback state_name(event_type(), eventContent :: term(), data :: data()) ::
            event_handler_result(state_name())

Note: no docs, even though they're properly attached to the chunk entry, because we have this discrepancy of state_name vs StateName.

Maybe it should be state_name in the chunk, that'd definitely solve the discrepancy, however looking at https://erlang.org/doc/man/gen_statem.html I think referring to it as Module:StateName/3 was on purpose, both the module name and function name vary.

If it makes sense to keep as StateName in the chunk, also worth mentioning that we won't be able to document such callback with EDoc given

-callback StateName(Foo) -> ok.

won't compile. We'd probably need to add support for a @name tag to overwrite how it's stored in the chunk.

-callback state_name(enter, OldState, Data).
%% @name StateName      (we document callbacks _after_ the definition.)

If we keep it as state_name in the chunk, maybe we do the reverse, store how the name should be displayed in chunk entry metadata.

lib/ex_doc/retriever.ex Outdated Show resolved Hide resolved
actual_def = actual_def(name, arity, kind)
doc_line = anno_line(anno)
annotations = annotations_from_metadata(metadata)
signature = if signature == [], do: nil, else: Enum.join(signature, " ")
Copy link
Member Author

@wojtekmach wojtekmach Feb 5, 2021

Choose a reason for hiding this comment

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

@josevalim I noticed that we ignored signature from the chunk entry for both callbacks and types so I thought I'd default to it. Was there a particular reason we didn't do it before? If this is the way to go, I'm happy to make sure Elixir is emitting the signature in the chunk.

Copy link
Member

Choose a reason for hiding this comment

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

It is on purpose to keep the chunks lean. If there is no signature, we fetch it from the AST. I would keep it as is for now.

@wojtekmach wojtekmach merged commit 9e52ce0 into master Feb 5, 2021
@wojtekmach wojtekmach deleted the wm-callbacks branch February 5, 2021 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants