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

Fix for missing nominal case in autolink_spec #1965

Merged
merged 1 commit into from
Nov 20, 2024

Conversation

lucioleKi
Copy link
Contributor

@lucioleKi lucioleKi commented Nov 20, 2024

Small fix for missing nominal case in autolink_spec.

Maybe the release will need to be updated as well?

@josevalim
Copy link
Member

Thank you! Is there a test we could add here?

@lucioleKi
Copy link
Contributor Author

What didn't match was something like this. It crashed when I build docs for OTP. I have difficulty writing a test that works at this moment, perhaps due to my limited knowledge about the syntax. I'm not sure how to build a term like this in the test.
{:attribute, [file: "/otp/lib/stdlib/src/erl_anno.erl", location: {122, 2}], :nominal, {:line, {:type, {122, 20}, :non_neg_integer, []}, []}}

After the nominal PR is merged, the following test could be added at line 578 test/language/erlang_test.exs to test for integration:

test "OTP nominal", c do
      assert autolink_extra("`t:erl_anno:location/0`", c) ==
               ~s|<a href="https://www.erlang.org/doc/apps/stdlib/erl_anno.html#t:location/0"><code class="inline">erl_anno:location/0</code></a>|

@josevalim
Copy link
Member

You can add the test today and wrap it in a conditional:

if System.otp_release() >= "28" do
  test "OTP nominal", c do
      assert autolink_extra("`t:erl_anno:location/0`", c) ==
               ~s|<a href="https://www.erlang.org/doc/apps/stdlib/erl_anno.html#t:location/0"><code class="inline">erl_anno:location/0</code></a>|
  end
end

WDYT?

@lucioleKi lucioleKi force-pushed the nominal branch 2 times, most recently from 3553b26 to e161c19 Compare November 20, 2024 11:43
@garazdawi
Copy link
Contributor

From what I understand this PR fixes when creating docs for code like this:

-doc "abd".
-nominal foo() :: file:name().

So the testcase needs to compile that kind of erlang code for it to work. I would look how the tests for spec works, like this one:

  test "type", c do
      assert autolink_spec("-type foo() :: t().", c) ==
               ~s|foo() :: <a href="#t:t/0">t</a>().|
    end

https://github.com/elixir-lang/ex_doc/blob/v0.35.0/test/ex_doc/language/erlang_test.exs#L716C5-L719C8

Small fix for missing nominal case in autolink_spec.
@lucioleKi
Copy link
Contributor Author

It turns out that existing test cases are sufficient. I can test the new changes locally with OTP 28. Once the CI here is updated to test for 28, it will also show up here.

@josevalim josevalim merged commit 5ab94c8 into elixir-lang:main Nov 20, 2024
5 checks passed
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

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