Support desc_parameter outside of desc_parameterlist #9692

johnpmcconnell · 2021-10-01T16:38:58Z

Note: I am actively working on contributing this request. I just need some feedback and maybe a little guidance.

Is your feature request related to a problem? Please describe.

I'm working on improving on sphinxcontrib-prettyspecialmethods, and a roadblock I'm running into is that there's no good way to emulate the formatting generated by desc_parameter. Even if there were, I think that desc_parameter semantically represents what I'm working with (a parameter that ultimately goes to a function) even though I'm trying to represent the function using language syntax rather than the function itself.

Describe the solution you'd like

It seems like the most modest change is to just update the translators to behave sensibly when they encounter a desc_parameter outside of a desc_parameterlist.

I've already begun work doing this. I believe that all that's required is to make the parameter-list based state more consistent by setting it up in __init__ and resetting it on depart and detecting whether the writer is inside a parameter list before trying to write out separators.

Aside from reviewing whether my changes above make sense or if there are other use cases I need to consider, I also need some help identifying where to put tests and maybe what sorts of tests I should write. Since this change is primarily to support manipulating or generating the document tree directly in a way that existing directives don't currently support, it cannot be tested using any existing directives. I wasn't able to locate any tests that work directly with the document tree being fed into a writer or translator. What would be a reasonable way to test these changes? (My intention is to write the tests for failure and then rebase the commits above on top of them.)

Describe alternatives you've considered

Trying to use a desc_parameter without a parent list is not very useful in Sphinx currently. Examples:
- If the desc_parameter instance appears before any instance of desc_parameterlist, some translators throw an error due to a missing attribute on the translator itself. The attribute is set in visit_desc_parameterlist and does not appear in __init__.
- A previous desc_parameterlist's child separator will be reused for the separator, since this is only ever set in visit_desc_parameterlist.
- The writer inserts commas (or whatever the last separator is) both before and after the parameter (at the same time) in some cases, due to the logic depending on counts set in visit_desc_parameterlist.
  Note that these examples indicate that the behavior is somewhat unpredictable or at least surprising, since the output depends on the order of evaluation of nodes within the document tree.
I can't use the desc_parameterlist node because the parentheses are hard coded in the translators, and I'd like more control over the formatting of the separating symbols than it offers anyway. I suppose strictly speaking, the translator could be modified to give more flexibility to desc_parameterlist handling, but that seems like it would be a larger change with much wider reaching impact and potential for bugs and breaking other functionality. Given that the behavior for a desc_parameter outside of a desc_parameterlist is already unsupported, unpredictable, and not particularly useful with the present behavior, it is highly unlikely that anyone is depending on this behavior already, and that means this change is unlikely to break other extensions or customizations. As a bonus, simply enabling a bare desc_parameter will grant much greater flexibility to other extension authors and future development work on Sphinx itself than merely modifying desc_parameterlist behavior.
I cannot just use an emphasis node. Some translators do not apply emphasis when translating desc_parameter nodes. I wish to emulate desc_parameter's behavior exactly to ensure future consistency with Sphinx's output, and the best way to do that is to just actually use the class.

The text was updated successfully, but these errors were encountered:

jakobandersen · 2021-10-01T16:57:14Z

I'm not sure how to think about this yet. To better understand, can you provide some context to where you need a parameter outside a list? or rather it seems, where you need a list which should not be in parens?

There are other places when describing signatures where (comma-separated) lists are needed, with optional start and end markup (e.g., template parameter/argument lists in C++, or nested function parameter lists in both C and C++), where desc_parameter(list) is not enough.
There is also some longer-term formatting ideas at #1514 (comment).
So another idea would be to create two new nodes that generalizes the current ones, e.g., a desc_list and desc_listitem. That would ensure that nothing old gets broken, which may be an easier way forward.

8000

johnpmcconnell · 2021-10-01T17:11:39Z

@jakobandersen Python implements operators and some other syntax using protocol methods. The goal of the prettyspecialmethods extension (which I linked above) is to render those methods' signatures using the actual syntax they enable rather than as ordinary methods. So for example, you can implement the + operator using the __add__(self, other: MyType) method, and prettyspcialmethods seeks to render it as self + other: MyType. other here should be rendered using the same semantics as a parameter, including any type hinting and the associated type cross references (as provided by intersphinx). Or you can implement square bracket assignment using __setitem__(self, key: KeyType, value: ValueType), which would be rendered as self[key: KeyType] = value: ValueType, where key and value need to follow the semantics of parameters. A desc_list and desc_listitem generalization would not help with this, or at least using them would be extremely clunky. The typical use case of an extension like prettyspecialmethods is with autodoc, generating the description from the class code itself, but there's no reason why it couldn't also be used with manually constructed reStructuredText providing Python signatures like __setitem__(key: KeyType, value: ValueType).

As for nothing old getting broken, my changes cannot possibly break anything that Sphinx actually supports doing. The behavior is already broken, as I detailed above (even throwing actual exceptions in some cases). The changes are actually more like a fix than they are an addition. I'm only classifying this as a feature request because it's clear that this use case was just plain never anticipated, and the code was written assuming it wouldn't be attempted. The only way these changes could break something is if someone has written code relying on the broken behavior, which is as unsupported as using a bare desc_parameter in the first place. That's why it seems like the approach I describe would be the simplest and lowest impact way forward, at least to me.

jakobandersen · 2021-10-01T18:23:32Z

... the prettyspecialmethods extension (which I linked above) is to render those methods' signatures using the actual syntax they enable rather than as ordinary methods.

Pretty nice idea.

should be rendered using the same semantics as a parameter, including any type hinting and the associated type cross references (as provided by intersphinx).

Note that desc_parameter does not provide cross references. That is something that is done by inserting the right nodes as children. As far as I could find (by checking the visit_desc_parameter methods of the writers) it is just a fancy way of perhaps putting the child nodes in emphasis (and handle the comma if there are other parameter nodes). So if I understanding your use-case you need to do the same as at

sphinx/sphinx/domains/python.py

Lines 212 to 235 in 063dcfd

    
           node = addnodes.desc_parameter() 
        
           if param.kind == param.VAR_POSITIONAL: 
        
               node += addnodes.desc_sig_operator('', '*') 
        
               node += addnodes.desc_sig_name('', param.name) 
        
           elif param.kind == param.VAR_KEYWORD: 
        
               node += addnodes.desc_sig_operator('', '**') 
        
               node += addnodes.desc_sig_name('', param.name) 
        
           else: 
        
               node += addnodes.desc_sig_name('', param.name) 
        
           if param.annotation is not param.empty: 
        
               children = _parse_annotation(param.annotation, env) 
        
               node += addnodes.desc_sig_punctuation('', ':') 
        
               node += nodes.Text(' ') 
        
               node += addnodes.desc_sig_name('', '', *children)  # type: ignore 
        
           if param.default is not param.empty: 
        
               if param.annotation is not param.empty: 
        
                   node += nodes.Text(' ') 
        
                   node += addnodes.desc_sig_operator('', '=') 
        
                   node += nodes.Text(' ') 
        
               else: 
        
                   node += addnodes.desc_sig_operator('', '=') 
        
               node += nodes.inline('', param.default, classes=['default_value'], 
        
                                    support_smartquotes=False)

(and all the stuff in _parse_annotation, but instead of putting nodes in a addnodes.desc_parameter then put them in a nodes.emphasis.

johnpmcconnell · 2021-10-01T19:10:03Z

@jakobandersen As noted in the alternatives I considered, not all translators put emphasis on parameter nodes. It's translator specific. Some also support a noemph attribute to disable it (although I am not at all clear how this attribute gets added), and some invoke astext() and skip over the children. Furthermore, the HTML translators place a number of CSS classes in the HTML translator, some of which change styles based on appearing inside a parent element with the sig-param class or not. (Oh, and the HTML translator doesn't even apply sig-param, while the HTML5 one does.) I am also aware that, strictly speaking, the cross ref is provided by the child nodes (a pending_xref if memory serves), but as noted, some translators will not process it.

In a lot ways, delving into all this validates the point: reproducing desc_parameter's structure and styling is well beyond trivial. It is heavily ingrained throughout all the different components involved in converting reStructuredText into a destination format. No one should need to duplicate a lot of Sphinx's built in logic just to provide a modest extension that transforms the document tree a little. Duplicating this much logic is very complicated when it's even possible (and much of the behavior is not possible to duplicate consistently because it exists in areas that have different behaviors depending on how the end user is using Sphinx, like in the translators chosen based on output format), and it lacks the future proofing that just allowing the use of a desc_parameter would provide. Trying to duplicate it borders on hacky (if not crossing well into the territory). With the proposed change in place, no one extending Sphinx needs to know anything about all the logic that goes into rendering a desc_parameter and its children; they only need to be able to provide one with appropriate children.

I made a deep dive into trying to understand the logic surrounding parameters before I even considered making this request, as I hope the level of detail my responses provide demonstrate. Believe me when I say that providing this upstream change is very likely the simplest way to achieve my end goals. I do not make this proposal lightly. It was only with careful consideration, a very hard look at what was possible and what was not, and a thorough understanding about the impact it would have that I chose to submit this. I believe very strongly that this change will be a significant (even if niche) net positive for Sphinx as a whole, enabling not only my use case, but also simplifying a host of other use cases in the future, without negatively impacting existing users and extensions. Consistent, predictable, and expected (the opposite of surprising) behavior is nearly always a win, even and especially for the developers maintaining the code base.

jakobandersen · 2021-10-02T08:30:49Z

As noted in the alternatives I considered, not all translators put emphasis on parameter nodes. It's translator specific.

Ah, right, there is a bit of more stuff going on in some of them (text, man, and latex at least). I think I got a bit confused in your original post, as even though the Python classes are called "translator" I think I have only seen these components referred to as the "writer"s.
But indeed, allowing desc_parameter outside desc_parameterlist seems like the most reasonable approach. The current behaviour was made a long time ago and is simply under-documented.

Some also support a noemph attribute to disable it (although I am not at all clear how this attribute gets added), and some invoke astext() and skip over the children.

The noemph is added by whomever creates the node. Example:

sphinx/sphinx/domains/cpp.py

Line 2112 in a0e986c

param = addnodes.desc_parameter('', '', noemph=True)

So in your case you need to de

(Oh, and the HTML translator doesn't even apply sig-param, while the HTML5 one does.)

Well-written PRs are always welcome.

I am also aware that, strictly speaking, the cross ref is provided by the child nodes (a pending_xref if memory serves), but as noted, some translators will not process it.

My best guess is that the writers that short-circuits the child rendering do not supports links anyway (e.g., the text writer).

In a lot ways, delving into all this validates the point: reproducing desc_parameter's structure and styling is well beyond trivial. It is heavily ingrained throughout all the different components involved in converting reStructuredText into a destination format.

To my understanding the "only" thing being reused is the code in the methods visit_desc_parameter and depart_desc_parameter in the writers, but is enough that I think it is good if it can be reused.

I made a deep dive into trying to understand the logic surrounding parameters before I even considered making this request, as I hope the level of detail my responses provide demonstrate. Believe me when I say that providing this upstream change is very likely the simplest way to achieve my end goals.

Please note that there are often requests for changes to solve specific issues, and I'm merely trying to figure out what the best way forward for Sphinx as a whole is.

Your linked changes seems reasonable to me so I suggest making a PR for more detailed review, also by @tk0miya. It would be great if you could expand the documentation of desc_parameterlist and desc_parameter to better explain their use.

johnpmcconnell · 2021-10-03T14:37:17Z

@jakobandersen Thank you.

Regarding the sig-param class, I was presuming that was intentional (although I did not investigate why; front end is not my specialty), but if it's not, I'll gladly include a fix. I was just noting it as a writer specific difference that would be hard to replicate.

Regarding the terminology, thanks for the clarification. I was trying to be specific to be more clear, but it backfired.

Please note that there are often requests for changes to solve specific issues, and I'm merely trying to figure out what the best way forward for Sphinx as a whole is.

My apologies if it seemed like I was implying otherwise. I was just trying to make it clear that I am also giving all the due weight to those same concerns, that I am not trying to just ram through something for my own short term benefit at the expense of the project's long term.

Your linked changes seems reasonable to me so I suggest making a PR for more detailed review, also by @tk0miya. It would be great if you could expand the documentation of desc_parameterlist and desc_parameter to better explain their use.

Will do. However, I would like to add some tests for the behavior before I take up even more of your time. Can you provide any advice on how to approach adding them? I could well be missing something, but I haven't seen any tests that feed a document tree directly into the writers. I could potentially try to plug into the existing build tests (such as test_build_html.py and the like), but since there's no reStructuredText directives that emit a bare desc_parameter, I'd probably need to build some kind of customized directive or transform into the test root's conf.py. I'm not looking for detailed instructions; just a pointer to where I should be looking or if I need to add something new.

johnpmcconnell added the type:enhancement enhance or introduce a new feature label Oct 1, 2021

AA-Turner added this to the some future version milestone Sep 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Support desc_parameter outside of desc_parameterlist #9692

Support desc_parameter outside of desc_parameterlist #9692

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Support desc_parameter outside of desc_parameterlist #9692

Support desc_parameter outside of desc_parameterlist #9692

Comments

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!