-
Notifications
You must be signed in to change notification settings - Fork 13
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
rfc13: fix formatting of C API docs #399
Conversation
Problem: openpmix no longer provides PMI-1 compatibility libraries but the PMI-1 doc mentions them. These never worked right and were eventually removed from openpmix. In addition, they were not required by the pmix specification. Drop that mention.
Problem: PMI-1 API documentation formats function parameters as literals, rendered in red monospace. Use :c:var:`var` instead. Fix up a couple of other minor formatting problems along the way.
Problem: Read the Docs has deprecated builds without a config file https://blog.readthedocs.com/migrate-configuration-v2/ Add a minimal .readthedocs.yaml config as documented in the above link.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Nice improvement (I didn't know about :c:var
and :func:
, I bet there are lots of other docs that could use this update).
@@ -72,8 +72,7 @@ protocol versions. Apart from version negotiation and the common | |||
fundamentals, PMI version 2 a different protocol and not covered here. | |||
|
|||
PMIx ("x" for exascale, from the OpenMPI community) is a separate effort | |||
that provides compatible PMI version 1 and 2 APIs but uses an incompatible | |||
wire protocol. PMIx is not covered here. | |||
that is not covered here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated to this PR, but I noticed a typo below this change:
PMIX ("X" for extension), is as set ...
Should be "is a set"?
spec_13.rst
Outdated
@@ -274,7 +274,7 @@ Initialization | |||
.. c:function:: int PMI_Init (int *spawned) | |||
|
|||
Initialize the PMI library for this process. Upon success, the value | |||
of ``spawned`` (boolean) SHALL bet set to (1) if this process was created | |||
of :c:var:`spawned` (boolean) SHALL bet set to (1) if this process was created | |||
by :c:func:`PMI_Spawn_multiple`, or (0) if not. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the difference between, :func:
and :c:func:
used here? (Above PMI_Spawn_multiple
is referenced with plain :func:
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't see any difference in the rendering but we should be consistent. Let me see if I can dig a little deeper in the docs and see what the default domain (correct term?) is for :func:
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah we should probably just set the default domain to c
in the document and drop the explicit uses.
https://www.sphinx-doc.org/en/master/tutorial/describing-code.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, good find. You know, I looked around for a bit and had trouble finding the correct documentation for this. I wonder if there's somewhere we should keep a reference to RFC best practices or something? Another RFC? 🤷
(maybe it hasn't risen to that level yet, but I find reStructuredText and sphinx to be fairly complicated and usually just refer to existing examples when writing docs, so apologies in advance when I mess this up in the future)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK to set MWP?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure!
Problem: sentence about PMIX contains a typo. Fix the typo.
Problem: RFC 13 inconsistently uses a :c: domain prefix in front of code directives. Set the default domain to c, then drop the domain prefixes from all occurrences of the directives function::, :func:, and :var:. https://www.sphinx-doc.org/en/master/tutorial/describing-code.html
Problem: PMI-1 API documentation formats function parameters as literals, rendered in red monospace.
Use
:c:var:
instead.Fix up a couple of other minor formatting problems along the way.