ex_doc icon indicating copy to clipboard operation
ex_doc copied to clipboard

Warn on invalid format inside backticks in links

Open eksperimental opened this issue 3 years ago • 5 comments

[old Erlang child specification](`t:supervisor.child_spec()`)

will generate

<a href="`t:supervisor.child_spec()`">old Erlang child specification</a>

I find myself making this kind of mistakes, and ExDoc not warnings about it. I think if Exdoc ends up generating a link that starts with a backtick, it should emit a warning.

~Well. Actually i think the format should be supported to avoid this kind of mistake, and render it without the backticks.~

eksperimental avatar May 19 '22 19:05 eksperimental

We should warn for invalid syntax indeed.

josevalim avatar May 19 '22 19:05 josevalim

After years of working with ExDoc and Elixir, and being heavily involved in documentations, I have problems we this feature.

The problem mentioned above it is unrelated to the use of the backtick, it is because I was missing a colon, it should have been t:: with a double colon because the Erlang module is :supervisor

[old Erlang child specification](`t::supervisor.child_spec()`)

I tried different combinations, and only number 7 prints out the intended URL, and none of the others emit a warning.

- 1 [Text](t:supervisor.child_spec/0)
- 2 [Text](t:supervisor.child_spec())
- 3 [Text](`t:supervisor.child_spec/0`)
- 4 [Text](`t:supervisor.child_spec()`)

- 5 [Text](t::supervisor.child_spec/0)
- 6 [Text](t::supervisor.child_spec())
- + 7 [Text](`t::supervisor.child_spec/0`)
- 8 [Text](`t::supervisor.child_spec()`)

There may be room for improvement.

eksperimental avatar May 19 '22 19:05 eksperimental

What I would expect is ExDoc to emit a warning in every case and suggest a solution to correct the problem if possible. ie. to add backticks, to add missing color, to replace arguments with arity

eksperimental avatar May 19 '22 20:05 eksperimental

I would start with backticks only and move forward with more warnings if no false positives show up.

josevalim avatar May 19 '22 20:05 josevalim

+1 for emitting a warning and NOT generating the link, returning a href with backticks is obviously a bug. Let's ship that and then in the future we can iterate on it, trying to be more helpful.

wojtekmach avatar May 19 '22 20:05 wojtekmach

Hey! I did some experimentation with this and I think one approach for this case would be:

  • If we fail to parse something inside backticks as a reference, we warn it as an invalid reference and remove the link from it.

I think this will work fine because as I understand, if we cannot find parse something as a reference, we assume it to be a link and just return it inside the link markdown block, but since backticks surrounding links are invalid CommonMark, we can check for their presence and emit an "malformed expression" warn. Any thoughts?

Also I've noticed that invalid mix tasks are warning but not removing the link (e.g: [foo](`mix foo`) == <a href="`mix foo`">, is this the expected behavior? If not, I'm willing to open another pull request for removing the link on invalid mix tasks before implementing this one.

viniciusmuller avatar Jun 17 '23 20:06 viniciusmuller

Do we remove the link in other cases? Or is the proposal to always remove? That also works for me but harder to discover (a broken link will eventually break).

agree on warning when there are backtics and they don’t point anywhere.

josevalim avatar Jun 17 '23 20:06 josevalim

Do we remove the link in other cases? Or is the proposal to always remove?

Currently only links for hidden functions are removed.

From what I understand the idea is to never emit links with backlinks, since they'll always be broken.

What is currently causing them to be emitted is the fact that whenever we find something under backticks and fail to parse it, we return it as a link, thus creating the broken links. Thus I think an invalid reference inside backticks would fall into an "invalid/malformed reference" category, and we could warn and remove the link since it would be generated with backticks.

viniciusmuller avatar Jun 17 '23 20:06 viniciusmuller

Sounds good to me!

josevalim avatar Jun 17 '23 21:06 josevalim