-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Improve cross-reference documentation #12944
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
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
As a short feedback, I find this listing not too helpful for the following reasons:
The only missing items there are manpage, pep, rfc. But possibly they should also get a section on this page?
Given the toc, I suggest that we can live without that list. But if you want to keep it, I suggest a bit of rework based on the above comments.
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.
Gone: 5eb68b2
The intent was to highlight the breath of roles that support cross referencing, but was perhaps ill-conceived.
A
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.
@timhoffm
typo:
should be:
In this case less is more. I feel the TOC is repetitive by always starting with the verb "Cross-referencing XYZ" but the corresponding headings work well reading through the document. Adding more intro text under each heading would be a mistake because it would add clutter and waste the reader's time. (As a comparison: did you ever notice how the first half of most videos should be skipped because it just repeats an intro we already know? So lets skip the formalism of adding intros under the headings.)
Untangling scope overlap of the Cross-referencing syntax doc with the Roles doc should be posted separately as an issue. It seems like a complex decision.
@mgeier We've had a number of users asking for help about ambiguous error messages caused by
:any:
when later something changes in the code (and they were using shortened syntax in the qualified name for example). Personally I always pinpoint the exact role to reduce ambiguity for myself and the reader -this also helps keep error messages more accurate- hence I don't usedefault_role
.I only write cross-references in the docstring when I absolutely must.
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.
@electric-coder
Well, that's your users' loss!
My docs are full of cross-references, see e.g. https://python-sounddevice.readthedocs.io/en/0.5.0/api/convenience-functions.html
Maybe you don't like writing them because they are so clunky when specifying the exact role?
Maybe you should do your users a favor and consider using
default_role = 'any'
?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.
A suitable default role depends on the type of documentation (size, how much and which kind of cross-links do you need, etc.) For example at matplotlib, we use the
py:obj
default role as a good middle-ground. We have a lot of code references and being able to write them often with just adding backticks keeps the doc sources very readable (particularly useful for docstrings that are often also used in plain text). For Python objects there is little ambiguity because the naming is determined by the code and you almost always want to have the function/class name exactly as is in the rendered docs.We keep other references explicit, e.g. using
:doc:
or:ref:
to prevent ambiguity. Also, by their nature, they are often longer and break the flow of reading anyway, so that adding the explicit prefixes does not make them significantly more clunky than they already are.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.
That is a good point, thanks for mentioning it!
When I experimented with this many years ago,
py:obj
had a severe limitation: it didn't allow me to use parentheses like this:`my_function()`
. But it worked (and still works) fine withany
.That was the main reason why I used
any
in my documentations and I subsequently completely forgot aboutpy:obj
... until now.I just checked, and this limitation seems to have been lifted in the meantime!
Therefore, I now agree that
:any:
is a niche application and most Sphinx users should choosedefault_role = 'py:obj'
in most of their (Python-based) projects!I'm also doing that in the project I used as an example above: spatialaudio/python-sounddevice#567