Fix Erlang Scanning & Warnings #1818
Conversation
This makes it so that we do not have to deal with parsing Erlang using the Elixir parser, things that are keywords in Elixir are not in Erlang and vice versa. The most recent example I came across was `c:do/1` which is valid in Erlang but not in Elixir.
|
Elixir should warn for types I think. @wojtekmach, maybe it is worth implementing that when you implement |
| @@ -432,7 +413,7 @@ defmodule ExDoc.Autolink do | |||
| {:type, _visibility} -> | |||
| case config.language.try_builtin_type(name, arity, mode, config, original_text) do | |||
| nil -> | |||
| if mode == :custom_link do | |||
| if mode == :custom_link or config.language == ExDoc.Language.Erlang do | |||
There was a problem hiding this comment.
Is this check here to avoid warning in Elixir? If so, you can go ahead and warn. :)
There was a problem hiding this comment.
Yes it is, but if I remove it a lot (~20) of tests break (or not break, but they start to emit warnings to stdout).
There was a problem hiding this comment.
Ok, @wojtekmach please look into it afterwards then. :)
Ideally we want to avoid hardcoding the language here, an option would be better, but since this is temporary, it is fine by me. :)
| {:regular_link, _module_visibility, :undefined} when not same_module? -> | ||
| {:regular_link, _module_visibility, :undefined} | ||
| when not same_module? and | ||
| (config.language != ExDoc.Language.Erlang or kind == :function) -> |
There was a problem hiding this comment.
Same here, feel free to let it warn on Elixir, assuming it is one of m:, t: or c:.
|
I found a bug in how warnings are reported from extra files, please don't merge until I've fixed it. |
garazdawi
force-pushed
the
lukas/fix-erlang-warnings-and-scanning
branch
from
5d20aec to
774aacd
Compare
|
Turns out that it was an issue in Feel free to merge whenever you feel like. |
garazdawi
force-pushed
the
lukas/fix-erlang-warnings-and-scanning
branch
from
71b36b8 to
db1c726
Compare
|
Thank you!
Good call, will do! |
|
@garazdawi I think after this PR your OTP branch with ExDoc+markdown is now crashing and I was able to narrow it down to ExDoc emitted warnings for docs defined in .hrl files. ExDoc gets the file as .erl and tries to render a line in it that doesn't exist, it should look into .hrl file. Unfortunately I couldn't reproduce this in a test yet. Our markdown doc tests are going through the legacy edoc machinery in test_helper and I think ideally we'd have another code path where we extract things from |
|
Yes, I'm aware. I've fixed it in a local branch together with some other fixes. The problem happens when we have doc entries with different file anno than the moduledoc and then when ExDoc tries to warn it takes the snippet from the wrong file. The branch with my current fixes is: https://github.com/garazdawi/ex_doc/commits/lukas/fix-erlang-doc-support/ and the fix for this particular issue is: garazdawi@0cfc40b |
|
FWIW EEP 48 expects the Anno node to point to the source and it has an optional metadata that points to the edit_url. I am not sure we use them both in ExDoc but we will be glad to accommodate! |
From what I've gathered, for ExDoc to work, Anno should point to the first line of the documentation. This is used when printing warnings by ExDoc etc. For Then ExDoc outputs a "view source" button that points to the source code, this is currently taken from the anno in the AST. (This only works for types in Erlang right now, yet another thing to fix :) ) Maybe it would make sense to have an "edit documentation" button that takes you to a page where you can update the docs? |
|
@garazdawi I created an issue to discuss Edit Documentation link: #1825. |
|
@garazdawi I cherry-picked all commits except using makeup_erlang fork (as we can't release to Hex with that) from https://github.com/garazdawi/ex_doc/commits/lukas/fix-erlang-doc-support/ on main. If you could send a PR to makeup_erlang we can release it too. If there's any other ExDoc things you'd like to land before the release, let us know too! |
|
Great! I've meant to open a PR for weeks now, but other things have always gotten in the way. I will send a pr to makeup_erlang. I don't have anything right now lined up for ExDoc, but I don't doubt that as we start converting more and more of our docs and other people start to look at it there will be issues and inconsistencies that come up. |
|
makeup_erlang v0.1.3 also released! |
This PR does 3 things:
I'm a bit unsure about the last one, why is it that ExDoc Elixir does not warn for these?
Seems like there is very little chance of a false positive, and if there is one you can always silence it.