[WIP] Autolink revamp - Round 2 #896

eksperimental · 2018-09-12T16:50:31Z

Round 1 is here: [WIP] Autolink revamp #893

This PR doesn't change the API as requested by @josevalim in #893 (comment)

sourcelevel-bot · 2018-09-12T16:51:15Z

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-12T16:51:15Z

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-12T16:51:15Z

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:51:15Z

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).

Why is this function public too?

sorry I forgot to make it private

sourcelevel-bot · 2018-09-12T16:51:15Z

lib/ex_doc/formatter/html/autolink.ex

+            end
+
+          :function ->
+            cond do


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

josevalim · 2018-09-21T11:04:23Z

lib/ex_doc/formatter/html/autolink.ex

+  #                     with a token
+  @doc false
+  @spec link(String.t(), language, kind, keyword) :: String.t()
+  def link(string, language, kind, options) do


Why is this function public?

forgot to make it private

josevalim · 2018-09-21T11:04:42Z

lib/ex_doc/formatter/html/autolink.ex

+    |> link_process(:preprocess, options[:preprocess?])
+    |> link(language, kind, :custom, options)
+    |> link(language, kind, :normal, options)
+    |> link_process(:postprocess, options[:preprocess?])


Wrong option here. It seems we do not have tests for this then?

Sorry, what is wrong here?

josevalim · 2018-09-21T11:05:29Z

lib/ex_doc/formatter/html/autolink.ex

+    end
+
+    term
+  end


Please remove debugging code.

josevalim · 2018-09-21T11:08:46Z

lib/ex_doc/formatter/html/autolink.ex

+  end
+
+  @doc false
+  # Replaces all backticks inside the text of custom links with @backtick_token.


Did we do pre-processing before? Why do we need it now?

We didn't. But it fixes the bug of linking inside the custom text. Before it was only working if either the opening backtick is touching the opening bracket, or the closing backtick is touching the closing bracket.

This wouldn't pass before IIRC.

assert Autolink.erlang_functions("[the `:erlang.apply/2` function](other.html)") == "[the `:erlang.apply/2` function](other.html)"

So this behaviour was inconsistent. I think it worked for local and Elixir functions and it didn't work for the rest

josevalim · 2018-09-21T11:14:18Z

lib/ex_doc/formatter/html/autolink.ex

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


I don't like that we are converting structured list into unstructured one. My suggestion is to keep using a map internally everywhere.

josevalim · 2018-09-21T11:14:51Z

lib/ex_doc/formatter/html/autolink.ex

+    case link_type do
+      :normal ->
+        fn all, match ->
+          replacement(all, language, kind, match, options)


I think we should merge replace_fun and replacement such that replace_fun already returns a function considering the link_type, language and kind. :)

Sorry, but after splitting replacement into two functions clauses I cannot see how to merge it with replace_fun

I ended up having something like this

defp replace_fun(language, kind, :normal, options) do fn all, match -> replacement(all, language, kind, match, options) end end defp replace_fun(language, kind, :custom, options) do fn all, text, match -> replacement(all, language, kind, match, text, options) end end # The heart of the autolinking logic defp replacement(string, language, kind, match, text \\ nil, options) defp replacement(string, :erlang, kind, match, text, options) do lib_dirs = Map.get(options, :lib_dirs, default_lib_dirs(:erlang)) pmfa = {_prefix, module, function, arity} = split_function(match) text = text || default_text(:erlang, kind, match, pmfa) if doc = module_docs(:erlang, module, lib_dirs) do case kind do :module -> "[#{text}](#{doc}#{module}.html)" :function -> "[#{text}](#{doc}#{module}.html##{function}-#{arity})" end else string end end defp replacement(string, :elixir, kind, match, text, options) do aliases = Map.get(options, :aliases, []) docs_refs = Map.get(options, :docs_refs, []) extension = Map.get(options, :extension, ".html") lib_dirs = Map.get(options, :lib_dirs, default_lib_dirs(:elixir)) locals = Map.get(options, :locals, []) module_id = Map.get(options, :module_id, nil) modules_refs = Map.get(options, :modules_refs, []) pmfa = {prefix, module, function, arity} = split_function(match) text = text || default_text(:elixir, kind, match, pmfa) elixir_docs = get_elixir_docs(aliases, lib_dirs) case kind do :module -> cond do match == module_id -> "[`#{match}`](#{match}#{extension}#content)" match in modules_refs -> "[`#{match}`](#{match}#{extension})" doc = module_docs(:elixir, match, lib_dirs) -> "[`#{match}`](#{doc}#{match}.html)" true -> string end :function -> cond do match in locals -> "[#{text}](##{prefix}#{enc_h(function)}/#{arity})" match in docs_refs -> "[#{text}](#{module}#{extension}##{prefix}#{enc_h(function)}/#{arity})" match in @basic_type_strings -> "[#{text}](#{elixir_docs}#{@basic_types_page})" match in @built_in_type_strings -> "[#{text}](#{elixir_docs}#{@built_in_types_page})" match in @kernel_function_strings -> "[#{text}](#{elixir_docs}Kernel#{extension}##{prefix}#{enc_h(function)}/#{arity})" match in @special_form_strings -> "[#{text}](#{elixir_docs}Kernel.SpecialForms#{extension}##{prefix}#{enc_h(function)}/#{ arity })" doc = module_docs(:elixir, module, lib_dirs) -> "[#{text}](#{doc}#{module}.html##{prefix}#{enc_h(function)}/#{arity})" true -> string end end

josevalim · 2018-09-21T11:16:26Z

Thanks @eksperimental! I have reviewed the code and now I can see with more clarity the improvements it brings. However, I still believe we have some extra work to do. In particular, I think the replacement function should be split and be concerned with at least erlang or elixir, instead of a large function that worries about all scenarios. ❤️ 💚 💙 💛 💜

sourcelevel-bot · 2018-10-03T00:35:29Z

lib/ex_doc/formatter/html/autolink.ex


-  @doc """
-  Helper function for autolinking elixir modules.
+  defp replacement(string, :elixir, kind, match, text, options) do


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

sourcelevel-bot · 2018-10-03T00:35:30Z

Ebert has finished reviewing this Pull Request and has found:

3 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/896.

eksperimental · 2018-10-03T00:38:22Z

@josevalim thank you for reviewing my code. I have implemented all the changes you suggested, except here and here

josevalim · 2018-10-06T09:41:09Z

❤️ 💚 💙 💛 💜

josevalim · 2018-10-06T10:26:18Z

@eksperimental I have applied the replace_fun suggestion in this commit.

Note I have also removed preprocess/postprocess, because they are testing implementation details and not behaviour. Namely, we don't care how we handle the backticks, as long as we handle it correctly, and that's what we need to test.

I believe now we can do the second part of your PR which is to rewrite the tests. Basically, right now we are testing elixir_functions, locals and so on, but the code does not actually call any of those functions. I think we need to change all of the Autolink tests to go exclusively through project_doc. We may need to make project_doc/4 public though to test it.

eksperimental · 2018-10-08T06:07:55Z

The tests for the preprocess/postprocess were there because I found bugs in my previous code.
Maybe I can conver those tests that were removed to use project_doc/4 to make sure we are dealing with backtick replacement correctly.

I will go through your commit whenever I getg some free time and get back if I have any comments.
thank you

wojtekmach · 2018-10-27T22:39:10Z

I just noticed that besides refactoring, we've introduced here a new feature: auto linking Erlang modules. It can introduce false positives, see: :file:

probably not a deal breaker but can be a bit jarring.

eksperimental · 2018-10-28T00:17:45Z

@wojtekmach great find!
I think we should revert it, or at least partially revert it, as it is going to be a big annoyance.
By partially I mean, we should remove this features from normal links and allow it only when called explicitly in custom links. WDYGT?

josevalim · 2018-10-28T00:21:49Z

I agree! -- *José Valimwww.plataformatec.com.br <http://www.plataformatec.com.br/>Founder and Director of R&D*

eksperimental · 2018-10-28T00:24:36Z

ok. let's here from others and I will provide a PR as soon as possible

josevalim · 2018-10-28T00:41:11Z

@eksperimental sweet. what do you have in mind? instead of a for comprehensions that builds @regexes, should we just list all combinations explicitly? Also, I think we should not support normal nor custom links for erlang modules. That would be a bit inconsistent.

josevalim · 2018-10-28T00:44:00Z

Pushed in 75d32db.

wojtekmach · 2018-10-28T06:38:58Z

Great, thanks everyone!

sourcelevel-bot bot reviewed Sep 12, 2018

View reviewed changes

eksperimental mentioned this pull request Sep 12, 2018

[WIP] Autolink revamp #893

Closed

eksperimental force-pushed the autolink_revamp2 branch from a0fb3b0 to 098481a Compare September 12, 2018 16:53

josevalim reviewed Sep 21, 2018

View reviewed changes

eksperimental added 2 commits October 3, 2018 06:10

Autolink revamp

0be1206

Implement suggestions by @josevalim

b7a264a

eksperimental force-pushed the autolink_revamp2 branch from 098481a to b7a264a Compare October 3, 2018 00:34

sourcelevel-bot bot reviewed Oct 3, 2018

View reviewed changes

josevalim merged commit 3319b05 into elixir-lang:master Oct 6, 2018

josevalim mentioned this pull request Oct 6, 2018

Rewrite Autolink suite to go exclusively through project_doc #904

Closed

josevalim mentioned this pull request Dec 9, 2018

(last one) + CONTRIBUTING.md #936

Closed

+                  end
+                  term
+                end

[WIP] Autolink revamp - Round 2 #896

[WIP] Autolink revamp - Round 2 #896

Uh oh!

Conversation

eksperimental commented Sep 12, 2018

Uh oh!

sourcelevel-bot bot Sep 12, 2018

Choose a reason for hiding this comment

Uh oh!

sourcelevel-bot bot Sep 12, 2018

Choose a reason for hiding this comment

Uh oh!

sourcelevel-bot bot Sep 12, 2018

Choose a reason for hiding this comment

Uh oh!

sourcelevel-bot bot Sep 12, 2018

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sourcelevel-bot bot Sep 12, 2018

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

sourcelevel-bot bot Oct 3, 2018

Choose a reason for hiding this comment

Uh oh!

sourcelevel-bot bot commented Oct 3, 2018

Uh oh!

eksperimental commented Oct 3, 2018

Uh oh!

josevalim commented Oct 6, 2018

Uh oh!

josevalim commented Oct 6, 2018

Uh oh!

eksperimental commented Oct 8, 2018

Uh oh!

wojtekmach commented Oct 27, 2018

Uh oh!

eksperimental commented Oct 28, 2018

Uh oh!

josevalim commented Oct 28, 2018 via email

Uh oh!

eksperimental commented Oct 28, 2018

Uh oh!

josevalim commented Oct 28, 2018

Uh oh!

josevalim commented Oct 28, 2018

Uh oh!

wojtekmach commented Oct 28, 2018

eksperimental Oct 3, 2018 •

edited

Loading

josevalim commented Sep 21, 2018 •

edited

Loading