Improve `supervisor` specs and docs #8015

Maria-12648430 · 2024-01-11T14:09:47Z

Prompted by discussions on the Erlang Forum, I took a look at the supervisor documentation and found some ways to improve it (or so I hope). However, this PR does not really improve the issue that lead to the confusion as discussed in the linked Forum post, as the roots for this lie deeper.

I'll put some more inline comments on what I did any why.

github-actions · 2024-01-11T14:10:49Z

CT Test Results

2 files 93 suites 34m 8s ⏱️
2 017 tests 1 969 ✅ 48 💤 0 ❌
2 326 runs 2 276 ✅ 50 💤 0 ❌

Results for commit 1f66c14.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

elbrujohalcon

😍

vkatsuba · 2024-01-11T14:18:33Z

🚀

Maria-12648430 · 2024-01-11T14:12:23Z

lib/stdlib/doc/src/supervisor.xml

@@ -401,8 +401,101 @@ child_spec() = #{id => child_id(),             % mandatory
        only. A map is preferred; see more details
        <seeerl marker="#sup_flags">above</seeerl>.</p></desc>
    </datatype>
+    <datatype>
+      <name name="sup_name"/>


This type has been introduced in order to pull it and the accompanying documentation out of start_link/2,3. Inspiration has been taken from the related server_name() types in gen_server and gen_statem.

I think it makes sense to have this type, it makes it flexible for this abstractions.

Maria-12648430 · 2024-01-11T14:13:21Z

lib/stdlib/doc/src/supervisor.xml

    <datatype>
      <name name="sup_ref"/>
+      <desc>


Inspiration for this type has been taken from the related server_ref() types in gen_server and gen_statem.
A description of sup_ref has been located in start_child/2 (but not in other functions using this type, like terminate_child/2, restart_child/2 etc), but I think it is much better placed here, and has therefore been removed from the start_child/2 function docs.

Yes something that we might consider is if proc_lib (that is the main building block) should have some
register_name type that all the behaviors and supervisors (that are implemented as gen_servers) could refer to. But I think this is an improvement over what we have and motivate by the behaviour modules current documentation.

Maria-12648430 · 2024-01-11T14:27:38Z

lib/stdlib/doc/src/supervisor.xml

@@ -534,38 +627,26 @@ child_spec() = #{id => child_id(),             % mandatory
    </func>

    <func>
-      <name name="start_child" arity="2" since=""/>


The docs for this function conflated both variants of the function, namely the one that accepts a child spec as second argument which can only be used for one_for_one, one_for_all and rest_for_one supervisors, and the one that accepts a list of additional start args as second argument which can only be used for simple_one_for_one supervisors.

Accordingly, the description had several passages like "(unless the supervisor is a simple_one_for_one supervisor, see below)" and "For a simple_one_for_one supervisor...".

This is a bit confusing IMO, since you always have to have a kind of filter running on in your head, increasing cognitive load 😅

For this reason, I split the function documentation in two, one for each of the two clauses, and thereby one for each kind/class of supervisors that it will work with, and how.

I agree that current formulation is hard to read. However clause_i will not be possible to use with the new
documentation format from #8026 so we have to come up with an alternative solution. clause_i is sometimes convenient but also quite error prone if the code changes it implementation adds/removes clauses. I am all for splitting it into "most supervisors" and "simple one for one supervisors"

I guessed as much when I saw #8026... This raises the question how this PR should be handled, though. Wait for #8026 to be merged and then rework it in the "new way"?

I think we do not want add new things that will need to be change once #8026 is merged. PR-8026 will not aim to be the final version, and other changes will be needed to make everything as we like it but we need to start somewhere.
Probably for now we will have to make some solution referring to if the function is called like x then otherwise if it is called like y then ... I have a similar problem in ssl docs but have not yet had time to make a solution.

No, what I mean is, how should I proceed with this PR in general? The most sensible way seems to be to "put it on hold" for now, wait for #8026 to be merged, then sort of write it again, this part and the others, according to the new way. This is ok for me, I don't mind 😉 WDYT?

Sure if you feel that is easier its planed for master anyway. I will put a stalled label on it, and remove it once #8026 is merged.

I think this is the best course, yes :)

Maria-12648430 · 2024-01-11T14:31:38Z

lib/stdlib/doc/src/supervisor.xml

+        <marker id="start_child-not_simple_one_for_one"/>
+        <note>
+          <p>This version of <c>start_child/2</c> can only be used with
+            <c>one_for_one</c>, <c>one_for_all</c> and <c>rest_for_one</c>
+            supervisors. For <c>simple_one_for_one</c> supervisors, the version
+            <seemfa marker="#start_child-simple_one_for_one"><c>start_child(SupRef, ExtraArgs)</c></seemfa>
+            must be used instead, see below.</p>
+        </note>


I'm not quite happy with this linking and the "see above"/"see below"... However, I couldn't find a way to link to the descriptions of specific function clauses, only to functions as a whole...

That said, there are some things that pop up once in a while for which I can't find any documentation for, namely the name_i and clause_i attributes. I use them here, but TBH I only picked that up from other documentation files where they are used. It works, and I can guess at what they do, but... 😅

lib/stdlib/doc/src/supervisor.xml

lib/stdlib/src/supervisor.erl

Maria-12648430 · 2024-01-11T14:37:30Z

@elbrujohalcon @vkatsuba thanks guys 🤗 I didn't expect reactions so quick TBH

IngelaAndin · 2024-01-17T10:09:42Z

lib/stdlib/doc/src/supervisor.xml

@@ -401,8 +401,101 @@ child_spec() = #{id => child_id(),             % mandatory
        only. A map is preferred; see more details
        <seeerl marker="#sup_flags">above</seeerl>.</p></desc>
    </datatype>
+    <datatype>
+      <name name="sup_name"/>


I think it makes sense to have this type, it makes it flexible for this abstractions.

IngelaAndin · 2024-01-17T10:15:27Z

lib/stdlib/doc/src/supervisor.xml

    <datatype>
      <name name="sup_ref"/>
+      <desc>


Yes something that we might consider is if proc_lib (that is the main building block) should have some
register_name type that all the behaviors and supervisors (that are implemented as gen_servers) could refer to. But I think this is an improvement over what we have and motivate by the behaviour modules current documentation.

IngelaAndin · 2024-01-17T10:39:50Z

lib/stdlib/doc/src/supervisor.xml

@@ -534,38 +627,26 @@ child_spec() = #{id => child_id(),             % mandatory
    </func>

    <func>
-      <name name="start_child" arity="2" since=""/>


I agree that current formulation is hard to read. However clause_i will not be possible to use with the new
documentation format from #8026 so we have to come up with an alternative solution. clause_i is sometimes convenient but also quite error prone if the code changes it implementation adds/removes clauses. I am all for splitting it into "most supervisors" and "simple one for one supervisors"

lib/stdlib/src/supervisor.erl

IngelaAndin · 2024-02-06T13:16:24Z

@Maria-12648430 Lukas has merged his doc PR #8026

Maria-12648430 · 2024-02-07T12:24:52Z

@Maria-12648430 Lukas has merged his doc PR #8026

Thanks for the heads-up @IngelaAndin, I'll get back to this one in the next couple of days then 👍

Co-authored-by: Jan Uhlig <[email protected]>

Maria-12648430 · 2024-02-08T14:10:57Z

@IngelaAndin I changed this PR to use the new format.

The rendered documentation for the start_child/2 function has an issue where a linebreak occurs right in the middle of ++. ~~I should bring that up with ex_doc I guess?~~ I raised an issue at ex_doc for this.

IngelaAndin · 2024-03-05T08:12:42Z

Thanks for the PR :)

Maria-12648430 changed the title ~~Improve supervisor specs and docs~~ Improve supervisor specs and docs Jan 11, 2024

elbrujohalcon approved these changes Jan 11, 2024

View reviewed changes

Maria-12648430 commented Jan 11, 2024

View reviewed changes

Maria-12648430 force-pushed the improve_supervisor_docs branch from ca84b58 to b5c1fe5 Compare January 11, 2024 18:42

rickard-green added the team:PS Assigned to OTP team PS label Jan 15, 2024

IngelaAndin self-assigned this Jan 16, 2024

IngelaAndin requested changes Jan 17, 2024

View reviewed changes

IngelaAndin added the stalled waiting for input by the Erlang/OTP team label Jan 23, 2024

IngelaAndin added waiting waiting for changes/input from author and removed stalled waiting for input by the Erlang/OTP team labels Feb 6, 2024

Improve supervisor specs and docs

1f66c14

Co-authored-by: Jan Uhlig <[email protected]>

Maria-12648430 force-pushed the improve_supervisor_docs branch from b5c1fe5 to 1f66c14 Compare February 8, 2024 12:44

IngelaAndin removed the waiting waiting for changes/input from author label Feb 12, 2024

IngelaAndin assigned u3s Feb 20, 2024

IngelaAndin requested review from IngelaAndin and u3s February 21, 2024 06:43

IngelaAndin approved these changes Feb 21, 2024

View reviewed changes

u3s removed their request for review February 28, 2024 09:23

IngelaAndin merged commit 002130b into erlang:master Mar 5, 2024
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve `supervisor` specs and docs #8015

Improve `supervisor` specs and docs #8015

Maria-12648430 commented Jan 11, 2024

github-actions bot commented Jan 11, 2024 •

edited

Loading

elbrujohalcon left a comment •

edited

Loading

vkatsuba commented Jan 11, 2024

Maria-12648430 Jan 11, 2024

IngelaAndin Jan 17, 2024

Maria-12648430 Jan 11, 2024

IngelaAndin Jan 17, 2024

Maria-12648430 Jan 11, 2024

IngelaAndin Jan 17, 2024

Maria-12648430 Jan 18, 2024

IngelaAndin Jan 22, 2024

Maria-12648430 Jan 23, 2024

IngelaAndin Jan 23, 2024

Maria-12648430 Jan 24, 2024

Maria-12648430 Jan 11, 2024

Maria-12648430 commented Jan 11, 2024

IngelaAndin Jan 17, 2024

IngelaAndin Jan 17, 2024

IngelaAndin Jan 17, 2024

IngelaAndin commented Feb 6, 2024

Maria-12648430 commented Feb 7, 2024

Maria-12648430 commented Feb 8, 2024 •

edited

Loading

IngelaAndin commented Mar 5, 2024

Improve supervisor specs and docs #8015

Improve supervisor specs and docs #8015

Conversation

Maria-12648430 commented Jan 11, 2024

github-actions bot commented Jan 11, 2024 • edited Loading

CT Test Results

Artifacts

elbrujohalcon left a comment • edited Loading

Choose a reason for hiding this comment

😍

vkatsuba commented Jan 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Maria-12648430 commented Jan 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IngelaAndin commented Feb 6, 2024

Maria-12648430 commented Feb 7, 2024

Maria-12648430 commented Feb 8, 2024 • edited Loading

IngelaAndin commented Mar 5, 2024

Improve `supervisor` specs and docs #8015

Improve `supervisor` specs and docs #8015

github-actions bot commented Jan 11, 2024 •

edited

Loading

elbrujohalcon left a comment •

edited

Loading

Maria-12648430 commented Feb 8, 2024 •

edited

Loading