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

DOC: Format role and directive cross-references in ReST format #12986

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

timhoffm
Copy link
Contributor

@timhoffm timhoffm commented Oct 8, 2024

Cross-references only consisted of the target name so far:
image
image

This PR formats the references as one would write the role / directive in ReST:

image
image

While the ReST formatting is a bit more noisy, using the actual ReST format helps to make the mental connection, in particular also for less experienced sphinx users, who may not have a clear idea what a directive is, but much more likely can connect tot the .. directive:: notation, which they have seen or written.

@jayaddison
Copy link
Contributor

In the context of the tutorials and instructional text, I like this change - as described, it does help to link the description of a directive with the output/syntax.

We use the :rst:dir:... notation in quite a few places around the documentation though, not only in in the tutorial code. Perhaps it'd look unusual and/or be quite repetitive for all of those to be displayed with this custom formatting?

Also, on a more technical note: I notice that a few domains override XRefRole to add customization; is adding an attribute to the base class preferable to a subclass here?

@timhoffm
Copy link
Contributor Author

timhoffm commented Oct 8, 2024

We use the :rst:dir:... notation in quite a few places around the documentation though, not only in in the tutorial code. Perhaps it'd look unusual and/or be quite repetitive for all of those to be displayed with this custom formatting?

:option: is not much more verbose than option, so that should be fine. I was a bit worried about .. directive:: being a bit too disruptive, but after implenting felt that it's quite good as well. Best skim through the rendered docs of the PR yourself and build your opinon based on it.

Configurability (use ReST / text only) is a bit difficult with roles. We could make so that we accept both :rst:dir:`directive` and `:rst:dir:`.. directive::` and render based on that difference. Though I'm not sure switching back and forth is a benefit, and it's more cumbersome for the author to write out the full ReST.

Also, on a more technical note: I notice that a few domains override XRefRole to add customization; is adding an attribute to the base class preferable to a subclass here?

No strong opinion. Generally, if a parameter is sufficient, I'd go with not creating a zoo of subclasses (also subclasses don't scale with combination of features), but if it's preferred, one could create specific XRefRole subclasses for different types of cross-references.

When going with the configurable-through-parameter-format variant, subclasses could be a bit cleaner.

@jayaddison
Copy link
Contributor

jayaddison commented Oct 8, 2024

:option: is not much more verbose than option, so that should be fine. I was a bit worried about .. directive:: being a bit too disruptive, but after implenting felt that it's quite good as well. Best skim through the rendered docs of the PR yourself and build your opinon based on it.

Ok, yep - after building that locally and inspecting the results (including pages where :rst:dir: is used particularly frequently), I agree - that seems fairly readable to me.

I was curious about the effect on screen readers for accessibility -- also given that this could potentially compound with #12943. However: it turns out that the markup for the literal blocks isn't announced at least by orca, and the additional audio/cognitive overhead from the punctuation characters didn't seem terrible to me (but maybe that's debatable, and I'm not a frequent screenreader user, so my opinion isn't well formed yet!).

Slightly off-topic: I did find this sentence somewhat confusing when spoken, but that can probably be resolved by rephrasing it

Each table can override the global style via :class: option, or .. rst-class:: for no-directive tables (cf. Tables).

Edit: attempt to clarify the description of screenreader behaviour re: literals

@jayaddison
Copy link
Contributor

aren't spoken

To clarify: I should have said that they aren't announced. orca does read the content of the literal blocks, but it doesn't add any spoken output when entering and exiting the literal text.

Copy link
Contributor

@jayaddison jayaddison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would have helped me while learning about some Sphinx/rST basics.

I'd suggest a second opinion about the choice between an attribute/subclassing for the implementation, because I'm fairly ambivalent about it (but do want to mention it, in case other people have maintenance-related opinions).

@timhoffm
Copy link
Contributor Author

timhoffm commented Oct 8, 2024

The importance of the decision also depends on whether we assume XRefRole class is public API, i.e. whether extensions derive from it. If not, we are able to easily refactor in the future when the need arises.

@electric-coder
Copy link

electric-coder commented Oct 9, 2024

Best skim through the rendered docs of the PR yourself and build your opinon based on it.

I've given this a lot of thought in the past especially when posting at SO. There's a difference between an "official doc" like the Sphinx documentation and an informal post in some Q&A somewhere. While I generally write .. directive:: and :role: when posting at SO I think it's a poor choice for the Sphinx docs.

IMO the change shouldn't be done, as said it becomes repetitive, then verbose and also redundant - when you say: ".. directive_XYZ:: directive" you've just said directive twice, once in prose and once in reST syntax. Finally, after that, it introduces one serious problem no "official doc" should have, that is: inconsistency. Users reading the Sphinx documentation expect opera and although meaning well the change sets a bad stylistic example that will annoy users in the long run... (Seen in this light there were sound reasons that still hold for this not having been done up until now.)

It may be tempting to spread coding artifacts into natural language prose, but this particular mixture breaks what should be the natural flow of the text. It's its own kind of XY problem: "lets not write correct prose because users can't read the reST between the lines "...

Let us be pragmatic, after an initial round of learning Sphinx a user can be expected to know the difference between a directive and a role. Any of the getting started guides explains it enough. So we might be aiming to be helpful towards users who haven't read anything or failed to understand even the most basic reST constructs - they'll have to try building the docs either way; and read the official docs attentively too! I don't think introducing this problem into the docs will lend any help in those cases.

That's my rational for giving a -1.


We could make so that we accept both :rst:dir:directive and :rst:dir:.. directive::` and render based on that difference.

But I'll make 1 more argument: If the PR is focused on adding this as a Sphinx feature instead of a change to the docs; I'll have to conservatively ask if this isn't going down the path of feature bloat? Is there a real need for this? Is there a real demand for this? If the PR took 500 hours would it be worth it?

But what's more: Isn't this changing/expanding basic and plain reST syntax as we've known it for years? (A role is a role, a directive a directive, and a cross-reference is a cross-reference...) Does the added complexity of changing the basic expectations come with any payoff considering POLA?

@timhoffm
Copy link
Contributor Author

timhoffm commented Oct 9, 2024

@electric-coder thanks for given your opinion. I hear your concerns, but disagree with a number of your arguments.

In the end, the question is: Is syntactic markup (of roles and/or directives) a net benefit for the reader? yes / no / it depends (which may warrant global or per usage configuability)

It is a weighted decision. The two central arguments are:

  • positive: recognizability (it's a link to an option) and mental connection the construct as used in the code
  • negative: adds a bit of clutter and may disturb reading flow

all other aspect are minor.

As a precedence, we use syntactic markup in add_function_parentheses.

There's one argument, though that I have to explicitly reject:

Let us be pragmatic, after an initial round of learning Sphinx a user can be expected to know the difference between a directive and a role.

I strongly believe, we should not go towards a RTFM attitude. It's not user-friendly, and it doesn't work. From my experience, only very few developers go and learn Sphinx from the ground up. Documentation isn't their primary goal and they don't invest extra time into learning its concepts. They just fiddle through and possibly have learned from example that one can write .. note:: or :func:`somefunc` , but I assume a significant fraction doesn't know the terms "directive" and "option" or which one is which. I believe it's a big win if we can make the documentation more understandable for non-sphinx experts.

@electric-coder
Copy link

electric-coder commented Oct 10, 2024

Esteemed @timhoffm you are welcome sir!

  1. Is syntactic markup (of roles and/or directives) a net benefit for the reader?

    The formulation seems elegant but it's deceiving; worst it's not reST-tonic.

    Interpreted text

    (...) but the text itself is typically left alone.


    What you are proposing is a change to cross-reference syntax (this is what it must be called for clarity!):

    :domain:role:`title <target>`

    For the general interpreted text role definition your proposal is already deviating from the quoted general reST spec. What's used for these cases are custom roles. While cross-references are, already, custom roles; their syntax and functionality is plain and straightforward (...)

  2. Configurability (use ReST / text only) is a bit difficult with roles. We could make so that we accept both :rst:dir:`directive` and :rst:dir:`.. directive::` and render based on that difference.

    (...) so what's being proposed here is a complete departure from previous Sphinx cross-reference logic (lets emphasize this) by adding magic to the processing of the title (see the two issue links further below).

  3. As a precedence, we use syntactic markup in add_function_parentheses.

    This argument is again deceiving. Because add_function_parentheses is a doc-wide configuration

    • it is not an individual per-use role functionality;
    • it does not add implicit logic to cross-reference title processing (it's an explicit config).

    One must ask: why mess with the well established cross-reference syntax (:domain:role:`title <target>` ) when writing an explicit title is enough, e.g. (:domain:role:`..directive_name:: <target>` )? The one end being syntatic sugar by way of brevity, benefits are outweighed by inherent complexity of introducing a sole exception.


  1. I strongly believe, we should not go towards a RTFM attitude

    Colloquially called the: Have you stopped beating your wife? statement.

    It's not user-friendly, and it doesn't work.

    You can say that about mypy. I've only once or twice seen users request this feature you're proposing, but what I've seen are hundreds of posts asking to change cross-reference syntax/processing e.g. one, two, etc... Giving all kinds of reasonings and arguments (you would be surprised how deceivingly intricate these are to rebate with a counter-demonstration).

    In fact, I'm persuaded we need a super-thread to group requests asking for cross-reference syntax changes, because there's so many of them and they'll keep pouring in periodically. For contrast this extensive bug list affects beginner or expert alike. That's the real time sink with Sphinx in particular and Python in general (thinking of the 5.5k mypy issues). For example, that Sphinx still fails to comply with basic reST spec (this one being 7 yo) - now that should be a priority (because soon those bugs will be 10 yo) because it affects everyone and more is being publicized than is actually being delivered.

  2. Documentation isn't their primary goal

    Every issue I've linked (and the many others I've seen here and at SO) asking for changes in cross-reference syntax are based on an XY problem. That's the part where unpacking the argumentation becomes intricate. Sphinx's aim is to write documentation, it is not to make writing documentation seamless (there's an extension for that called autosummary) .

    Here resides the problem: you're proposing a fundamental syntax change (X) to make examples easier to write (Y) the end being to help beginners (Z). That's a triple XYZ problem conflation of what are separate issues.

  3. :rst:dir:`directive`

    Here the use case shows its narrowness. It only applies to the Sphinx docs and Sphinx extension docs; its use being limited to the reST domain because general purpose docs use domain declarations and cross-references to those declarations, they do not need to meta reference reST per se so the proposed :rst: domain specific implicit title transformation rule to distinguish between :role: and .. directive:: introduces an exception to a much more general syntactic rule for the sake of a single domain and two constructs.

    This is one of Sphinx's potential pandora's boxes: (the demonstration I was getting at) that would be to special case the general cross-reference syntax over N domains for M (=directives+roles) cases (times L language specific features like inheritance in issue 11434). That's the most general formulation of this recurring problematic (well illustrated by the previously linked issues 11434 and 12863).

  4. "All the king's horses and all the king's men / Couldn't put Humpty together again"

    What would opening the cross-reference syntax pandora box look like? Something like issue 11991 for the autodoc extension. Where the need to cater to language specific special cases creates an ever growing, ongoing, endless time wasting problem.

    Many of the linked bugs in issue 11991 are game-breaking and even for an experienced user it's hard if not impossible to work around them. (The entire ecosystem is affected.)

So in a nutshell no. It could make sense to add a system-wide config (like add_function_parentheses) but not a magic implicit title processing rule as the sole exception to how cross-references are processed in general. I don't see a real tradeoff justifying the pernicious introduction of a single special-case exception (that can already be achieved by existing means). And what's more, I think it's a non-sequitur to claim this will help users write reST without learning reST; or to introduce a functionality aimed at rewriting the official Sphinx docs before a debate is held focused on whether the Sphinx docs should be written that way, or not.

@chrisjsewell
Copy link
Member

Changing XRefRole is definitely beyond the scope of this PR this, and personally I think it is overkill

who may not have a clear idea what a directive is

If they don't know this, then they should not be using directives 🤷
the documentation should provide a clear definition of a directive, if it doesn't already

@timhoffm
Copy link
Contributor Author

@chrisjsewell I'm unclear on the implecation of your comment. Are you objecting the syntactic formatting of role and/or directive links, or the way it is implemented by making XRefRole configurable?

@chrisjsewell
Copy link
Member

Are you objecting the syntactic formatting of role and/or directive links, or the way it is implemented by making XRefRole configurable?

both 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants