[WIP] Autolink revamp #893

eksperimental · 2018-09-08T17:05:53Z

This started trying to fix a bug and I had to end up revamping a big part of the Autolink module.
I couldn't set a custom link to a built-in type,

[term()](`t:term/0`)

wouldn't work, while

range()

would work.

So, long story short, custom link only work for Elixir functions and locals,
but nothing else. Now custom links can be used for anything like built-in types and erlang functions

It was extremely hard to grasp what was going on with the regexes, so I created a SystaxRule module.

Please let me know what you guys think

sourcelevel-bot · 2018-09-08T17:06:41Z

lib/ex_doc/formatter/html/autolink.ex

+
+  def re(:f, :erlang) do
+    ~r{
+      # TODO: revise the erlang rules for function names


sourcelevel-bot · 2018-09-08T17:06:42Z

lib/ex_doc/formatter/html/autolink.ex

+  def re(:m, :erlang) do
+    ~r{
+      :                                       # prefix
+      # TODO: revise the erlang rules for module names 


sourcelevel-bot · 2018-09-08T17:06:42Z

test/ex_doc/formatter/html/syntax_rule_test.exs

+      mfa = re_source(:mfa, :erlang)
+
+      assert "[`example`](`:mod.example/1`)" ==
+               run(re({:custom_link, mfa}, :markdown), "[`example`](`:mod.example/1`)") |> hd()


Use a function call when a pipeline is only one function long

@ebert I find your suggestion specially for long lines like this not useful

sourcelevel-bot · 2018-09-08T17:06:42Z

test/ex_doc/formatter/html/syntax_rule_test.exs

+      mfa = re_source(:mfa)
+
+      assert "[`example`](`Mod.example/1`)" ==
+               run(re({:custom_link, mfa}, :markdown), "[`example`](`Mod.example/1`)") |> hd()


Use a function call when a pipeline is only one function long

sourcelevel-bot · 2018-09-08T17:06:42Z

test/ex_doc/formatter/html/syntax_rule_test.exs

+
+    test "mfa" do
+      assert ":my_erlang_module.is_fun!/123" ==
+               run(re(:mfa, :erlang), "`:my_erlang_module.is_fun!/123`") |> hd()


Use a function call when a pipeline is only one function long

sourcelevel-bot · 2018-09-08T17:06:43Z

test/ex_doc/formatter/html/syntax_rule_test.exs

+
+    test "functions" do
+      assert "fun" == run(re(:f), "`fun/1`") |> hd()
+      assert "is_FUN_99?" == run(re(:f), "`is_FUN_99?/1`") |> hd()


Use a function call when a pipeline is only one function long

sourcelevel-bot · 2018-09-08T17:06:43Z

test/ex_doc/formatter/html/syntax_rule_test.exs

+    end
+
+    test "functions" do
+      assert "fun" == run(re(:f), "`fun/1`") |> hd()


Use a function call when a pipeline is only one function long

sourcelevel-bot · 2018-09-08T17:06:43Z

test/ex_doc/formatter/html/syntax_rule_test.exs

+    test "modules" do
+      refute run(re(:m), "module.fun/1")
+      assert "Module" == run(re(:m), "Module.fun/1") |> hd()
+      assert "Module.Some" == run(re(:m), "Module.Some.fun/1") |> hd()


Use a function call when a pipeline is only one function long

sourcelevel-bot · 2018-09-08T17:06:43Z

test/ex_doc/formatter/html/syntax_rule_test.exs

+
+    test "modules" do
+      refute run(re(:m), "module.fun/1")
+      assert "Module" == run(re(:m), "Module.fun/1") |> hd()


Use a function call when a pipeline is only one function long

sourcelevel-bot · 2018-09-08T17:06:43Z

lib/ex_doc/formatter/html/autolink.ex

@@ -1,10 +1,162 @@
+defmodule ExDoc.SyntaxRule do


Modules should have a @moduledoc tag.

I am afraid testing this is pretty much testing implementation detail. Can we please inline this module inside the current autolink one and erase those tests?

eksperimental · 2018-09-08T17:12:11Z

It also fixed this bug.

[the `:erlang.apply/2` function](other.html)

Eventhough it was announced in the docs, it didn't work.
using backticks as part of the custom link test only worked when the backtick is at the beginning and/or the end of the text, due to a limitation of how regexes work.

sourcelevel-bot · 2018-09-08T17:34:05Z

lib/ex_doc/formatter/html/autolink.ex

+        module_id: module_id,
+        locals: locals
+      })
+      |> Map.to_list()


Use a function call when a pipeline is only one function long

sourcelevel-bot · 2018-09-08T17:34:06Z

lib/ex_doc/formatter/html/autolink.ex

+
+  @spec re_source(atom, language) :: String.t()
+  def re_source(name, language \\ :elixir) do
+    re(name, language) |> Regex.source()


Use a function call when a pipeline is only one function long

sourcelevel-bot · 2018-09-08T17:34:06Z

lib/ex_doc/formatter/html/autolink.ex

+      `\s*                                      # leading backtick
+      (#{function_re_source})                   # CAPTURE 1
+      \s*`                                      # trailing backtick
+      (?!`)                                     


There should be no trailing white-space at the end of a line.

sourcelevel-bot · 2018-09-08T17:34:06Z

lib/ex_doc/formatter/html/autolink.ex

-        "[#{text}](#{module}#{extension}##{prefix}#{enc_h(function)}/#{arity})"
+    case language do
+      :erlang ->
+        cond do


Cond statements should contain at least two conditions besides true.

sourcelevel-bot · 2018-09-08T17:34:06Z

lib/ex_doc/formatter/html/autolink.ex

+  @doc false
+  # The heart of the autolinking logic
+  @spec replacement(String.t(), language, kind, String.t(), keyword) :: String.t()
+  def replacement(string, language, kind, match, text \\ nil, options) do


Function is too complex (CC is 22, max is 9).

What should I do here?

sourcelevel-bot · 2018-09-08T17:34:06Z

lib/ex_doc/formatter/html/autolink.ex

+            end
+
+          :function ->
+            cond do


Function body is nested too deep (max depth is 2, was 3).

I don't think I can fix this, it will loose readability. What shall I do about it?

eksperimental · 2018-09-08T19:31:36Z

lib/ex_doc/formatter/html/autolink.ex

-          "[#{text}](#{elixir_docs}Kernel.SpecialForms" <>
-            "#{extension}##{prefix}#{enc_h(function)}/#{arity})"
+  @doc false
+  def erlang_functions(string) when is_binary(string) do


I have made these set of functions no longer accesible in the documentation, as I consider they are not longer necessary. They have been kept because all the tests rely on them.
We should improve the docs of the newly introduce function that replaces them link/4.

Those functions can be removed. We had them out to ease testing.

josevalim · 2018-09-09T07:05:10Z

lib/ex_doc/formatter/html/autolink.ex

+  @type kind :: :function | :module
+  @type link_type :: :normal | :custom
+
+  random_string = 1..10_000_000 |> Enum.random() |> Integer.to_charlist(36)


Let's pick a random string. Having it being a different value every time we compile the code will make it hard to spot bugs.

josevalim · 2018-09-09T07:09:08Z

Thanks @eksperimental for the great work. Unfortunately this PR is doing two things at once, refactoring the code and changing the user surface API. This makes it hard to review and exposes new contracts we are not sure we should expose. Can we please do the refactoring first and keep the user API the same? Then we can have a discussion on what becomes public and what becomes private. Thanks!

- Rename ExDoc.SyntaxRule to ExDoc.Formatter.HTML.SyntaxRule - Fix syntax rules for normal links - Add internal option `:preprocess?` to Autolink.link/4, which is set to `true` by default - Fix Autolink.link_everything/2 to only preprocess and postprocess just once - Add corner-cases section in text, including a few ones

eksperimental · 2018-09-10T14:15:27Z

@josevalim what API changes are you refering to? I'm confused bc above you mention that we can remove the functions locals, erlang-functions, elixir-functions, etc

sourcelevel-bot · 2018-09-10T16:16:26Z

lib/ex_doc/formatter/html/autolink.ex

+
+  defp re(:f, :erlang) do
+    ~r{
+      # TODO: revise the erlang rules for function names


sourcelevel-bot · 2018-09-10T16:16:27Z

lib/ex_doc/formatter/html/autolink.ex

+  defp re(:m, :erlang) do
+    ~r{
+      :                                       # prefix
+      # TODO: revise the erlang rules for module names 


sourcelevel-bot · 2018-09-10T16:16:27Z

lib/ex_doc/formatter/html/autolink.ex

  @kernel_function_strings for {f, a} <- kernel_exports, do: "#{f}/#{a}"
  @special_form_strings for {f, a} <- special_form_exports, do: "#{f}/#{a}"

+


There should be no more than 1 consecutive blank lines.

sourcelevel-bot · 2018-09-10T16:16:27Z

lib/ex_doc/formatter/html/autolink.ex

+  defp re(:m, :erlang) do
+    ~r{
+      :                                       # prefix
+      # TODO: revise the erlang rules for module names 


There should be no trailing white-space at the end of a line.

sourcelevel-bot · 2018-09-10T16:16:27Z

lib/ex_doc/formatter/html/autolink.ex

+  @doc false
+  # The heart of the autolinking logic
+  @spec replacement(String.t(), language, kind, String.t(), keyword) :: String.t()
+  def replacement(string, language, kind, match, text \\ nil, options) do


Function is too complex (CC is 21, max is 9).

josevalim · 2018-09-10T22:40:55Z

@eksperimental we can move them but in a separate commit. The public API should stay the same while refactoring. This means we shouldn't expose the link function and the existing API should remain documented. Thanks!

The regex was too greedy

sourcelevel-bot · 2018-09-12T16:43:36Z

test/ex_doc/formatter/html/autolink_test.exs

+      """
+
+      refute Autolink.preprocess(string) === string
+      assert Autolink.preprocess(string) |> Autolink.postprocess() === string


Use a function call when a pipeline is only one function long

sourcelevel-bot · 2018-09-12T16:43:36Z

lib/ex_doc/formatter/html/autolink.ex

-      match in @kernel_function_strings ->
-        "[#{text}](#{elixir_docs}Kernel#{extension}##{prefix}#{enc_h(function)}/#{arity})"
+          :function ->
+            cond do


Function body is nested too deep (max depth is 2, was 3).

sourcelevel-bot · 2018-09-12T16:43:37Z

Ebert has finished reviewing this Pull Request and has found:

6 possible new issues (including those that may have been commented here).

You can see more details about this review at https://ebertapp.io/github/elixir-lang/ex_doc/pulls/893.

eksperimental · 2018-09-12T16:51:16Z

Closing in favor of #896

Autolink revamp

b2c39fa

eksperimental requested a review from wojtekmach September 8, 2018 17:05

eksperimental changed the title ~~[WIP]Autolink revamp~~ [WIP] Autolink revamp Sep 8, 2018

sourcelevel-bot bot reviewed Sep 8, 2018

View reviewed changes

Make Ebert happy

3baf5f6

sourcelevel-bot bot reviewed Sep 8, 2018

View reviewed changes

eksperimental force-pushed the autolink_revamp branch 2 times, most recently from 8c1c380 to 71205cb Compare September 8, 2018 17:52

Make Ebert happy round 2

13c2c33

eksperimental force-pushed the autolink_revamp branch from 71205cb to 13c2c33 Compare September 8, 2018 18:00

eksperimental commented Sep 8, 2018

View reviewed changes

josevalim reviewed Sep 9, 2018

View reviewed changes

Implement suggestions by @josevalim

7b4d1ee

sourcelevel-bot bot reviewed Sep 10, 2018

View reviewed changes

remove warning

8c2d3d0

eksperimental force-pushed the autolink_revamp branch from ec5f50a to 8c2d3d0 Compare September 10, 2018 16:35

Fix bug replacing backtick inside custom links

8e42565

The regex was too greedy

sourcelevel-bot bot reviewed Sep 12, 2018

View reviewed changes

eksperimental mentioned this pull request Sep 12, 2018

[WIP] Autolink revamp - Round 2 #896

Merged

eksperimental closed this Sep 12, 2018

eksperimental deleted the autolink_revamp branch August 19, 2020 07:43

		@kernel_function_strings for {f, a} <- kernel_exports, do: "#{f}/#{a}"
		@special_form_strings for {f, a} <- special_form_exports, do: "#{f}/#{a}"

[WIP] Autolink revamp #893

[WIP] Autolink revamp #893

Uh oh!

Conversation

eksperimental commented Sep 8, 2018

Uh oh!

sourcelevel-bot bot Sep 8, 2018

Choose a reason for hiding this comment

Uh oh!

sourcelevel-bot bot Sep 8, 2018

Choose a reason for hiding this comment

Uh oh!

sourcelevel-bot bot Sep 8, 2018

Choose a reason for hiding this comment

Uh oh!

eksperimental Sep 8, 2018

Choose a reason for hiding this comment

Uh oh!

sourcelevel-bot bot Sep 8, 2018

Choose a reason for hiding this comment

Uh oh!

sourcelevel-bot bot Sep 8, 2018

Choose a reason for hiding this comment

Uh oh!

sourcelevel-bot bot Sep 8, 2018

Choose a reason for hiding this comment

Uh oh!

sourcelevel-bot bot Sep 8, 2018

Choose a reason for hiding this comment

Uh oh!

sourcelevel-bot bot Sep 8, 2018

Choose a reason for hiding this comment

Uh oh!

sourcelevel-bot bot Sep 8, 2018

Choose a reason for hiding this comment

Uh oh!

sourcelevel-bot bot Sep 8, 2018

Choose a reason for hiding this comment

Uh oh!

josevalim Sep 9, 2018

Choose a reason for hiding this comment

Uh oh!

eksperimental commented Sep 8, 2018

Uh oh!

sourcelevel-bot bot Sep 8, 2018

Choose a reason for hiding this comment

Uh oh!

sourcelevel-bot bot Sep 8, 2018

Choose a reason for hiding this comment

Uh oh!

sourcelevel-bot bot Sep 8, 2018

Choose a reason for hiding this comment

Uh oh!

sourcelevel-bot bot Sep 8, 2018

Choose a reason for hiding this comment

Uh oh!

sourcelevel-bot bot Sep 8, 2018

Choose a reason for hiding this comment

Uh oh!

eksperimental Sep 8, 2018

Choose a reason for hiding this comment

Uh oh!

sourcelevel-bot bot Sep 8, 2018

Choose a reason for hiding this comment

Uh oh!

eksperimental Sep 8, 2018

Choose a reason for hiding this comment

Uh oh!

eksperimental Sep 8, 2018

Choose a reason for hiding this comment

Uh oh!

josevalim Sep 9, 2018

Choose a reason for hiding this comment

Uh oh!

josevalim Sep 9, 2018

Choose a reason for hiding this comment

Uh oh!

josevalim commented Sep 9, 2018

Uh oh!

eksperimental commented Sep 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

eksperimental commented Sep 10, 2018 •

edited

Loading