Warning based on refs #1233

eksperimental · 2020-07-23T06:33:38Z

Update warning messages based on Ref's visibility Messages are more accurate now. Deal with private types

This PR spawned from #1205 tackling most of the issues addressed there.

eksperimental · 2020-07-23T06:34:01Z

Please do not squash the commits

test/ex_doc/autolink_test.exs

lib/ex_doc/autolink.ex

test/ex_doc/formatter/html_test.exs

test/ex_doc/refs_test.exs

lib/ex_doc/autolink.ex

test/ex_doc/autolink_test.exs

test/ex_doc/formatter/html_test.exs

wojtekmach · 2020-07-25T22:49:01Z

I tested this branch against Elixir docs and got these warnings:

warning: documentation references type "Exception.optional/1" but it is undefined lib/elixir/lib/exception.ex:15: t:Exception.t/0 warning: documentation references type "URI.optional/1" but it is undefined lib/elixir/lib/uri.ex:115: URI.decode_query/2 warning: documentation references type "URI.optional/1" but it is undefined lib/elixir/lib/uri.ex:115: URI.decode_query/2 (...)

eksperimental · 2020-07-25T23:18:12Z

I tested this branch against Elixir docs and got these warnings:

warning: documentation references type "Exception.optional/1" but it is undefined lib/elixir/lib/exception.ex:15: t:Exception.t/0 warning: documentation references type "URI.optional/1" but it is undefined lib/elixir/lib/uri.ex:115: URI.decode_query/2 warning: documentation references type "URI.optional/1" but it is undefined lib/elixir/lib/uri.ex:115: URI.decode_query/2 (...)

It is the last missing piece to be implemented
#1219

eksperimental · 2020-08-04T23:51:59Z

Rebased to master.
This is ready for review @wojtekmach
Please do not squash the commits.

wojtekmach · 2020-09-15T06:48:10Z

test/ex_doc/autolink_test.exs

+ assert warn(~m"[an unknown task](`mix unknown.task`)") =~
+ "documentation references \"mix unknown.task\" but it is undefined"
+
+ assert_unchanged(~m"[an unknown task](`mix unknown.task`)")


a lot of these new assertions were already present in the warnings tests, could you remove the duplicates and move remaining ones to the warnings test?

I have moved all the warnings to its own test, regarding the non warning ones. the Elixir ones are only checked in the test named "special case links" and the Erlang ones are not checked anywhere. I would say it is better to leave them in this test

test/ex_doc/autolink_test.exs

lib/ex_doc/autolink.ex

wojtekmach · 2020-09-15T07:04:46Z

test/ex_doc/autolink_test.exs

@@ -1,5 +1,5 @@
 defmodule ExDoc.AutolinkTest do
- use ExUnit.Case, async: true
+ use ExUnit.Case


if we test all warnings in that one test we could run it concurrently again, should we do that?

Tests do not run concurrently within the same module, regardless of the :async option, so that wouldn't be the limitation, if that is what you mean.
Concurrency was disabled because there are two ExUnit cases that are capturing :stderr, the other one is ExDoc.CLITest

It is supported on Elixir v1.11 though so let's leave the option in and push everyone to more recent Elixir versions.

It is supported on Elixir v1.11 though so let's leave the option in and push everyone to more recent Elixir versions.

@josevalim Sorry, but what is supported in v1.11?

if we test all warnings in that one test we could run it concurrently again, should we do that?

Do we really have a limitation having different tests emitting a warning since they are in the same test case , since they do not run concurrently within the same test case?

josevalim · 2020-09-15T17:31:22Z

Concurrent captures of stderr. -- *José Valimhttps://dashbit.co/ <https://dashbit.co/>*

eksperimental · 2020-09-15T17:38:24Z

Concurrent captures of stderr. -- José Valimhttps://dashbit.co/ https://dashbit.co/

Oh.. I was not aware of. Is this documented somewhere? I couldn't find any reference nor in the ExUnit.CaptureIO docs neither in the CHANGELOG

josevalim · 2020-09-15T17:41:37Z

Maybe it is in 1.10 then? -- *José Valimhttps://dashbit.co/ <https://dashbit.co/>*

eksperimental · 2020-09-15T18:08:41Z

Maybe it is in 1.10 then? -- José Valimhttps://dashbit.co/ https://dashbit.co/

is this the PR? elixir-lang/elixir#9527

josevalim · 2020-09-15T18:13:55Z

Yes!

eksperimental · 2020-09-15T21:29:24Z

Sorry, I accidentally closed this, and I am unable to reopen it. Can anybody reopen it?

josevalim · 2020-09-15T21:36:56Z

Apparently I cannot reopen it either.

eksperimental · 2020-09-15T21:44:19Z

I accidentally force pushed to a version that was a the same commit as master, and GitHub automatically closed it.

wojtekmach

Looks good to me, thank you. I added a few comments. Please also rebase with latest master.

lib/ex_doc/autolink.ex

lib/ex_doc/refs.ex

* Fine grain when to emit warning based on Ref's visibility and kind of link (custom or regular) * Improve building of extra and custom links

eksperimental · 2020-10-05T15:43:26Z

Looks good to me, thank you. I added a few comments. Please also rebase with latest master.

All suggestions implemented. And code has been rebased to master.

wojtekmach · 2020-10-06T08:46:08Z

Thanks!

eksperimental · 2020-10-06T11:25:53Z

Thank you @wojtekmach for reviewing this one

eksperimental force-pushed the warning_based_on_refs branch from be67210 to 143c75b Compare July 23, 2020 06:39

wojtekmach reviewed Jul 25, 2020

View reviewed changes

wojtekmach mentioned this pull request Jul 25, 2020

Change Autolink.warn/maybe_warn functions so they keep the original string provided in the documentation #1235

Merged

eksperimental force-pushed the warning_based_on_refs branch from fcc1fb6 to b577cda Compare July 25, 2020 22:25

wojtekmach reviewed Jul 25, 2020

View reviewed changes

test/ex_doc/formatter/html_test.exs Outdated Show resolved Hide resolved

eksperimental force-pushed the warning_based_on_refs branch from b577cda to f533827 Compare July 26, 2020 01:42

eksperimental requested a review from wojtekmach July 26, 2020 02:01

eksperimental force-pushed the warning_based_on_refs branch 3 times, most recently from f8e5e3b to c7a8e03 Compare August 4, 2020 23:31

eksperimental force-pushed the warning_based_on_refs branch from 686fbb2 to 16939df Compare August 8, 2020 17:06

eksperimental mentioned this pull request Aug 12, 2020

Feature Request: support :skip_reference_warnings_for #1192

Closed

wojtekmach reviewed Sep 15, 2020

View reviewed changes

eksperimental force-pushed the warning_based_on_refs branch 3 times, most recently from 6d3f749 to 16669a1 Compare September 15, 2020 16:59

eksperimental force-pushed the warning_based_on_refs branch from 16669a1 to ec8c885 Compare September 15, 2020 18:09

eksperimental force-pushed the warning_based_on_refs branch from ec8c885 to d436281 Compare September 15, 2020 19:11

eksperimental closed this Sep 15, 2020

eksperimental force-pushed the warning_based_on_refs branch from d436281 to 68b71f2 Compare September 15, 2020 19:12

eksperimental reopened this Sep 15, 2020

wojtekmach reviewed Oct 5, 2020

View reviewed changes

lib/ex_doc/autolink.ex Outdated Show resolved Hide resolved

lib/ex_doc/autolink.ex Outdated Show resolved Hide resolved

lib/ex_doc/refs.ex Outdated Show resolved Hide resolved

lib/ex_doc/refs.ex Outdated Show resolved Hide resolved

eksperimental added 7 commits October 5, 2020 10:25

Store private types to Refs with visibility set as :hidden

711300d

Fine grain warning messages based on Ref's visibility and kind of link

1c98dc2

* Fine grain when to emit warning based on Ref's visibility and kind of link (custom or regular) * Improve building of extra and custom links

Deal with required() and optional() in map types

a67c78c

Fix assert_unchanged

f7d8f05

Move warning tests to its own test

01968ab

Move hidden test to warning

e8389f4

Implement suggestions by @wojtekmach

3d75cc0

eksperimental force-pushed the warning_based_on_refs branch from a34b496 to 3d75cc0 Compare October 5, 2020 15:52

Update autolink.ex

aab5277

wojtekmach merged commit 4e647e6 into elixir-lang:master Oct 6, 2020

wojtekmach mentioned this pull request Oct 6, 2020

ExDoc cannot deal with literal map typespecs :optional and :required fields #1219

Closed

Warning based on refs #1233

Warning based on refs #1233

Uh oh!

Conversation

eksperimental commented Jul 23, 2020

eksperimental commented Jul 23, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wojtekmach commented Jul 25, 2020

eksperimental commented Jul 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

eksperimental commented Aug 4, 2020

wojtekmach Sep 15, 2020

Choose a reason for hiding this comment

eksperimental Sep 15, 2020 • 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!

Uh oh!

wojtekmach Sep 15, 2020

Choose a reason for hiding this comment

eksperimental Sep 15, 2020

Choose a reason for hiding this comment

josevalim Sep 15, 2020

Choose a reason for hiding this comment

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

Uh oh!

Choose a reason for hiding this comment

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

Uh oh!

Choose a reason for hiding this comment

josevalim commented Sep 15, 2020 via email

eksperimental commented Sep 15, 2020

josevalim commented Sep 15, 2020 via email

eksperimental commented Sep 15, 2020

josevalim commented Sep 15, 2020

eksperimental commented Sep 15, 2020

josevalim commented Sep 15, 2020

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

Uh oh!

wojtekmach left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eksperimental commented Oct 5, 2020

wojtekmach commented Oct 6, 2020

eksperimental commented Oct 6, 2020

Labels

3 participants

eksperimental commented Jul 25, 2020 •

edited

Loading

eksperimental Sep 15, 2020 •

edited

Loading

eksperimental Sep 15, 2020 •

edited

Loading

eksperimental Sep 15, 2020 •

edited

Loading

eksperimental commented Sep 15, 2020 •

edited

Loading